Skip to content

Conversation

@kijanowski
Copy link
Contributor

Motivation

pulsar-client runs conf/pulsar_tools_env.sh to set PULSAR_EXTRA_OPTS. If PULSAR_EXTRA_OPTS are given in the command line and contain -Xmx, it will be overridden by PULSAR_MEM.

Modifications

Exchange the order of applying env variables. First go the hardcoded PULSAR_MEM settings, then potentially the user settings.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

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 and label this PR (if you have committer privilege).

Need to update docs?

  • [ x] no-need-doc

Change in a shell script, which is not documented.

`pulsar-client` runs `conf/pulsar_tools_env.sh` to set `PULSAR_EXTRA_OPTS`. If `PULSAR_EXTRA_OPTS` are given in the command line and contain `-Xmx`, it will be overridden by `PULSAR_MEM`.
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 17, 2021
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

@hezhangjian
Copy link
Member

Why we need this feature, you can config both PULSAR_MEM and PULSAR_EXTRA_OPS
cc @lhotari @michaeljmarshall

@lhotari
Copy link
Member

lhotari commented Dec 18, 2021

Why we need this feature, you can config both PULSAR_MEM and PULSAR_EXTRA_OPS cc @lhotari @michaeljmarshall

@shoothzj I believe that this change improves consistency. The assumption is that you can override other options by setting them in PULSAR_EXTRA_OPTS. I made a similar improvement to consistency for bin/pulsar as part of PR #13025 .
Another point is that this change isn't harmful or risky.

@codelipenghui codelipenghui merged commit c9d8ecf into apache:master Dec 23, 2021
@kijanowski kijanowski deleted the patch-1 branch December 23, 2021 09:58
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Dec 29, 2021
…pache#13381)

### Motivation

`pulsar-client` runs `conf/pulsar_tools_env.sh` to set `PULSAR_EXTRA_OPTS`. If `PULSAR_EXTRA_OPTS` are given in the command line and contain `-Xmx`, it will be overridden by `PULSAR_MEM`.

### Modifications

Exchange the order of applying env variables. First go the hardcoded PULSAR_MEM settings, then potentially the user settings.
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants