Skip to content

Allow pulsar_tool_env.sh PULSAR_MEM to be Overridden - #15868

Merged
michaeljmarshall merged 1 commit into
apache:masterfrom
jabbaugh:pulsar_mem_tool_env
Jun 9, 2022
Merged

Allow pulsar_tool_env.sh PULSAR_MEM to be Overridden#15868
michaeljmarshall merged 1 commit into
apache:masterfrom
jabbaugh:pulsar_mem_tool_env

Conversation

@jabbaugh

@jabbaugh jabbaugh commented May 31, 2022

Copy link
Copy Markdown
Contributor

Motivation

The pulsar_tool_env.sh sets the PULSAR_MEM environment variable without allowing it to be overridden. When running an pulsar-admin function (e.g. running the kafka to pulsar connector) we can hit java memory issues without a way to change the memory settings. This PR resolves this issue.

Modifications

This change keeps the default value but allows PULSAR_MEM to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-not-needed
    (Please explain why)
    There is not currently documentation around the pulsar_tools_env.sh PULSAR_MEM setting. This change doesn't change the default behavior.

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 31, 2022
michaeljmarshall
michaeljmarshall approved these changes May 31, 2022
View reviewed changes

@michaeljmarshall michaeljmarshall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your contribution @jabbaugh! While we're making this change, it is probably worth adding the same override logic for the PULSAR_GC variable. Are you able to add that as well?

@michaeljmarshall michaeljmarshall added this to the 2.11.0 milestone May 31, 2022
@jabbaugh
jabbaugh force-pushed the pulsar_mem_tool_env branch from 69bd081 to dcedcb1 Compare May 31, 2022 19:21
@jabbaugh

jabbaugh commented May 31, 2022

Copy link
Copy Markdown
Contributor Author

@michaeljmarshall I have added the PULSAR_GC override as well.

michaeljmarshall
michaeljmarshall approved these changes May 31, 2022
View reviewed changes

@michaeljmarshall michaeljmarshall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaeljmarshall

michaeljmarshall commented May 31, 2022

Copy link
Copy Markdown
Member

@codelipenghui @lhotari @eolivelli - given that this change affects configuration for pulsar connectors, I think it's worth cherry picking it to 2.8, 2.9, and 2.10. Do you agree?

Jim Baugh
Allow PULSAR_MEM & PULSAR_GC to be Overridden in pulsar_tool_env.sh
5ef2aee 
The pulsar_tool_env.sh sets the PULSAR_MEM and PULSAR_GC environment variables without allowing them to be overridden.
This change keps the default values but allows PULSAR_MEM & PULSAR_GC to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.
@jabbaugh
jabbaugh force-pushed the pulsar_mem_tool_env branch from dcedcb1 to 5ef2aee Compare May 31, 2022 23:19
gaozhangmin
gaozhangmin approved these changes Jun 1, 2022
View reviewed changes

@gaozhangmin gaozhangmin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaeljmarshall
michaeljmarshall merged commit fa6288e into apache:master Jun 9, 2022
michaeljmarshall pushed a commit that referenced this pull request Jun 9, 2022
@jabbaugh @michaeljmarshall
Allow PULSAR_MEM & PULSAR_GC to be Overridden in pulsar_tool_env.sh (#…
12e4e61 
…15868)

The pulsar_tool_env.sh sets the PULSAR_MEM and PULSAR_GC environment variables without allowing them to be overridden.
This change keps the default values but allows PULSAR_MEM & PULSAR_GC to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

Co-authored-by: Jim Baugh <jim.baugh@oracle.com>

### Motivation
The pulsar_tool_env.sh sets the PULSAR_MEM environment variable without allowing it to be overridden. When running an pulsar-admin function (e.g. running the kafka to pulsar connector) we can hit java memory issues without a way to change the memory settings. This PR resolves this issue.

### Modifications
This change keeps the default value but allows PULSAR_MEM to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

### Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

### Does this pull request potentially affect one of the following parts:

*If `yes` was chosen, please highlight the changes*

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API: (no)
  - The schema: (no)
  - The default values of configurations: (no)
  - The wire protocol: (no)
  - The rest endpoints: (no)
  - The admin cli options: (no)
  - Anything that affects deployment: (no)

### Documentation

Check the box below or label this PR directly.

Need to update docs?
- [X] `doc-not-needed`
(Please explain why)
There is not currently documentation around the pulsar_tools_env.sh PULSAR_MEM setting. This change doesn't change the default behavior.

(cherry picked from commit fa6288e)
michaeljmarshall pushed a commit that referenced this pull request Jun 9, 2022
@jabbaugh @michaeljmarshall
Allow PULSAR_MEM & PULSAR_GC to be Overridden in pulsar_tool_env.sh (#…
43234c6 
…15868)

The pulsar_tool_env.sh sets the PULSAR_MEM and PULSAR_GC environment variables without allowing them to be overridden.
This change keps the default values but allows PULSAR_MEM & PULSAR_GC to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

Co-authored-by: Jim Baugh <jim.baugh@oracle.com>

### Motivation
The pulsar_tool_env.sh sets the PULSAR_MEM environment variable without allowing it to be overridden. When running an pulsar-admin function (e.g. running the kafka to pulsar connector) we can hit java memory issues without a way to change the memory settings. This PR resolves this issue.

### Modifications
This change keeps the default value but allows PULSAR_MEM to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

### Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

### Does this pull request potentially affect one of the following parts:

*If `yes` was chosen, please highlight the changes*

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API: (no)
  - The schema: (no)
  - The default values of configurations: (no)
  - The wire protocol: (no)
  - The rest endpoints: (no)
  - The admin cli options: (no)
  - Anything that affects deployment: (no)

### Documentation

Check the box below or label this PR directly.

Need to update docs?
- [X] `doc-not-needed`
(Please explain why)
There is not currently documentation around the pulsar_tools_env.sh PULSAR_MEM setting. This change doesn't change the default behavior.

(cherry picked from commit fa6288e)
michaeljmarshall pushed a commit that referenced this pull request Jun 9, 2022
@jabbaugh @michaeljmarshall
Allow PULSAR_MEM & PULSAR_GC to be Overridden in pulsar_tool_env.sh (#…
f1f1d4b 
…15868)

The pulsar_tool_env.sh sets the PULSAR_MEM and PULSAR_GC environment variables without allowing them to be overridden.
This change keps the default values but allows PULSAR_MEM & PULSAR_GC to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

Co-authored-by: Jim Baugh <jim.baugh@oracle.com>

### Motivation
The pulsar_tool_env.sh sets the PULSAR_MEM environment variable without allowing it to be overridden. When running an pulsar-admin function (e.g. running the kafka to pulsar connector) we can hit java memory issues without a way to change the memory settings. This PR resolves this issue.

### Modifications
This change keeps the default value but allows PULSAR_MEM to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

### Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

### Does this pull request potentially affect one of the following parts:

*If `yes` was chosen, please highlight the changes*

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API: (no)
  - The schema: (no)
  - The default values of configurations: (no)
  - The wire protocol: (no)
  - The rest endpoints: (no)
  - The admin cli options: (no)
  - Anything that affects deployment: (no)

### Documentation

Check the box below or label this PR directly.

Need to update docs?
- [X] `doc-not-needed`
(Please explain why)
There is not currently documentation around the pulsar_tools_env.sh PULSAR_MEM setting. This change doesn't change the default behavior.

(cherry picked from commit fa6288e)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 13, 2022
@jabbaugh @nicoloboschi
Allow PULSAR_MEM & PULSAR_GC to be Overridden in pulsar_tool_env.sh (a…
1146c9c 
…pache#15868)

The pulsar_tool_env.sh sets the PULSAR_MEM and PULSAR_GC environment variables without allowing them to be overridden.
This change keps the default values but allows PULSAR_MEM & PULSAR_GC to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

Co-authored-by: Jim Baugh <jim.baugh@oracle.com>

### Motivation
The pulsar_tool_env.sh sets the PULSAR_MEM environment variable without allowing it to be overridden. When running an pulsar-admin function (e.g. running the kafka to pulsar connector) we can hit java memory issues without a way to change the memory settings. This PR resolves this issue.

### Modifications
This change keeps the default value but allows PULSAR_MEM to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

### Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

### Does this pull request potentially affect one of the following parts:

*If `yes` was chosen, please highlight the changes*

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API: (no)
  - The schema: (no)
  - The default values of configurations: (no)
  - The wire protocol: (no)
  - The rest endpoints: (no)
  - The admin cli options: (no)
  - Anything that affects deployment: (no)

### Documentation

Check the box below or label this PR directly.

Need to update docs?
- [X] `doc-not-needed`
(Please explain why)
There is not currently documentation around the pulsar_tools_env.sh PULSAR_MEM setting. This change doesn't change the default behavior.

(cherry picked from commit fa6288e)
(cherry picked from commit f1f1d4b)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 13, 2022
@jabbaugh @nicoloboschi
Allow PULSAR_MEM & PULSAR_GC to be Overridden in pulsar_tool_env.sh (a…
0dce444 
…pache#15868)

The pulsar_tool_env.sh sets the PULSAR_MEM and PULSAR_GC environment variables without allowing them to be overridden.
This change keps the default values but allows PULSAR_MEM & PULSAR_GC to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

Co-authored-by: Jim Baugh <jim.baugh@oracle.com>

### Motivation
The pulsar_tool_env.sh sets the PULSAR_MEM environment variable without allowing it to be overridden. When running an pulsar-admin function (e.g. running the kafka to pulsar connector) we can hit java memory issues without a way to change the memory settings. This PR resolves this issue.

### Modifications
This change keeps the default value but allows PULSAR_MEM to be overridden which aligns with the
pulsar_env.sh file. This allows adjustments to be made to the memory settings when more memory is needed.

### Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

### Does this pull request potentially affect one of the following parts:

*If `yes` was chosen, please highlight the changes*

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API: (no)
  - The schema: (no)
  - The default values of configurations: (no)
  - The wire protocol: (no)
  - The rest endpoints: (no)
  - The admin cli options: (no)
  - Anything that affects deployment: (no)

### Documentation

Check the box below or label this PR directly.

Need to update docs?
- [X] `doc-not-needed`
(Please explain why)
There is not currently documentation around the pulsar_tools_env.sh PULSAR_MEM setting. This change doesn't change the default behavior.

(cherry picked from commit fa6288e)
(cherry picked from commit 12e4e61)
@poorbarcode poorbarcode mentioned this pull request Apr 6, 2023
Merged
4 tasks
poorbarcode added a commit that referenced this pull request Apr 11, 2023
@poorbarcode
[fix] [cli] Fix Broker crashed by too much memory usage of pulsar too…
4f503fd 
…ls (#20031)

### Motivation
After #15868, we allow `PULSAR_MEM` & `PULSAR_GC` to be overridden in `pulsar_tool_env.sh`.

Many users set `-Xms` to `2G` or larger in `PULSAR_MEM`, this will make the tools(such as `pulsar-admin`) cost a lot of memory, and if users execute `pulsar-admin` or another tool on the machine where the Broker is deployed, the current device will not have enough memory to allocate, resulting in a broker crash.

### Modifications

When `PULSAR_MEM` is overridden  in `pulsar_tool_env.sh`, delete parameter `-Xms`
poorbarcode added a commit that referenced this pull request Apr 11, 2023
@poorbarcode
[fix] [cli] Fix Broker crashed by too much memory usage of pulsar too…
728554d 
…ls (#20031)

### Motivation
After #15868, we allow `PULSAR_MEM` & `PULSAR_GC` to be overridden in `pulsar_tool_env.sh`.

Many users set `-Xms` to `2G` or larger in `PULSAR_MEM`, this will make the tools(such as `pulsar-admin`) cost a lot of memory, and if users execute `pulsar-admin` or another tool on the machine where the Broker is deployed, the current device will not have enough memory to allocate, resulting in a broker crash.

### Modifications

When `PULSAR_MEM` is overridden  in `pulsar_tool_env.sh`, delete parameter `-Xms`

(cherry picked from commit 4f503fd)
poorbarcode added a commit that referenced this pull request Apr 11, 2023
@poorbarcode
[fix] [cli] Fix Broker crashed by too much memory usage of pulsar too…
1fe05d5 
…ls (#20031)

### Motivation
After #15868, we allow `PULSAR_MEM` & `PULSAR_GC` to be overridden in `pulsar_tool_env.sh`.

Many users set `-Xms` to `2G` or larger in `PULSAR_MEM`, this will make the tools(such as `pulsar-admin`) cost a lot of memory, and if users execute `pulsar-admin` or another tool on the machine where the Broker is deployed, the current device will not have enough memory to allocate, resulting in a broker crash.

### Modifications

When `PULSAR_MEM` is overridden  in `pulsar_tool_env.sh`, delete parameter `-Xms`

(cherry picked from commit 4f503fd)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 11, 2023
@poorbarcode @nicoloboschi
[fix] [cli] Fix Broker crashed by too much memory usage of pulsar too…
0ab34c2 
…ls (apache#20031)

### Motivation
After apache#15868, we allow `PULSAR_MEM` & `PULSAR_GC` to be overridden in `pulsar_tool_env.sh`.

Many users set `-Xms` to `2G` or larger in `PULSAR_MEM`, this will make the tools(such as `pulsar-admin`) cost a lot of memory, and if users execute `pulsar-admin` or another tool on the machine where the Broker is deployed, the current device will not have enough memory to allocate, resulting in a broker crash.

### Modifications

When `PULSAR_MEM` is overridden  in `pulsar_tool_env.sh`, delete parameter `-Xms`

(cherry picked from commit 4f503fd)
(cherry picked from commit 1fe05d5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaozhangmin gaozhangmin gaozhangmin approved these changes
@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes
@codelipenghui codelipenghui Awaiting requested review from codelipenghui
@eolivelli eolivelli Awaiting requested review from eolivelli
@lhotari lhotari Awaiting requested review from lhotari

Assignees

@jabbaugh jabbaugh

Labels

area/config area/connector cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.8.4 release/2.9.3 release/2.10.1

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

3 participants

@jabbaugh @michaeljmarshall @gaozhangmin