Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix usage of PULSAR_EXTRA_OPTS/BOOKIE_EXTRA_OPTS in startup scripts #13025

Merged
merged 1 commit into from
Dec 1, 2021
Merged

Fix usage of PULSAR_EXTRA_OPTS/BOOKIE_EXTRA_OPTS in startup scripts #13025

merged 1 commit into from
Dec 1, 2021

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Nov 29, 2021 •
edited
Loading

Motivation

The changes in #11285 broke pulsar-perf and it fails with error

root@3b36a0ea475b:/pulsar# ./bin/pulsar-perf produce 
[0.000s][error][logging] Error opening log file 'logs/pulsar_gc_14.log': No such file or directory
[0.000s][error][logging] Initialization of output 'file=logs/pulsar_gc_%p.log' using options 'filecount=10,filesize=20M' failed.
Invalid -Xlog option '-Xlog:gc*:logs/pulsar_gc_%p.log:time,uptime:filecount=10,filesize=20M', see error log for details.
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

When analyzing the problem, it seems that the usage of PULSAR_EXTRA_OPTS/BOOKIE_EXTRA_OPTS environment variables isn't correctly used. It should be possible to use PULSAR_EXTRA_OPTS/BOOKIE_EXTRA_OPTS to add additional environment variables. This PR fixes that issue.

Modifications

  • don't combine BOOKIE_MEM, BOOKIE_GC and BOOKIE_GC_LOG in BOOKIE_EXTRA_OPTS
  • don't add PULSAR_GC_LOG to PULSAR_EXTRA_OPTS
  • make BOOKIE_GC_LOG default to PULSAR_GC_LOG for consistency
  • apply PULSAR_EXTRA_OPTS and BOOKIE_EXTRA_OPTS after other settings so that it is possible to override
    settings defined by PULSAR_MEM / PULSAR_GC / BOOKIE_MEM / BOOKIE_GC.

Documentation

  • no-need-doc

@lhotari
Fix environment variable assignment in startup scripts
6b18bab 
- don't combine variables in BOOKIE_EXTRA_OPTS
- don't add PULSAR_GC_LOG to PULSAR_EXTRA_OPTS
- make BOOKIE_GC_LOG default to PULSAR_GC_LOG for consistency
- apply PULSAR_EXTRA_OPTS and BOOKIE_EXTRA_OPTS after other settings so that it is possible to override
  settings defined by PULSAR_MEM / PULSAR_GC
@lhotari lhotari added the type/bug The PR fixed a bug or issue reported a bug label Nov 29, 2021
@lhotari lhotari self-assigned this Nov 29, 2021
@lhotari lhotari mentioned this pull request Nov 29, 2021
Closed
@github-actions
Copy link

@lhotari:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

@lhotari:Thanks for providing doc info!

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Nov 29, 2021
eolivelli
eolivelli approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

nicoloboschi
nicoloboschi approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

+1

MMirelli
MMirelli reviewed Nov 29, 2021
View reviewed changes
Copy link
Contributor

@MMirelli MMirelli left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli eolivelli merged commit f00ee7b into apache:master Dec 1, 2021
@eolivelli eolivelli added this to the 2.10.0 milestone Dec 1, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Dec 1, 2021
Merge branch 'up/master' into website/branch-2.6.1-part-5
f6b0a66 
* up/master: (75 commits)
  [website][upgrade]feat: website upgrade / docs migration - 2.5.1 Get Started/Concepts and Architecture/Pulsar Schema (apache#13030)
  Fix environment variable assignment in startup scripts (apache#13025)
  update 2.8.x (apache#13029)
  [Doc] add tips for Pulsar tools (apache#13044)
  Suggest to use tlsPort instead of deprecated TlsEnable (apache#13039)
  Integration tests for function-worker rebalance and drain operations. (apache#13058)
  fix(functions): missing runtime set in GoInstanceConfig (apache#13031)
  [pulsar-admin] Add get-replicated-subscription-status command for topic (apache#12891)
  [Broker] Consider topics in pulsar/system namespace as system topics (apache#13050)
  Fix typo: correct sizeUint to sizeUnit (apache#13040)
  fix-12894 (apache#12896)
  Don't attempt to delete pending ack store unless transactions are enabled (apache#13041)
  [Perf] Evaluate the current protocol version once (apache#13045)
  Fix Issue apache#12885, Unordered consuming case in Key_Shared subscription (apache#12890)
  [broker]Optimize topicMaxMessageSize with topic local cache. (apache#12830)
  [PIP-105] Part-2 Support pluggable entry filter in Dispatcher (apache#12970)
  [website] Modify admin-api-topic.md document (apache#12996)
  add missed import (apache#13037)
  [metadata] Add RocksdbMetadataStore (apache#12776)
  [C] Add pulsar_client_subscribe_multi_topics and pulsar_client_subscribe_pattern (apache#12965)
  ...

# Conflicts:
#	site2/website-next/docusaurus.config.js
#	site2/website-next/versioned_sidebars/version-2.6.1-sidebars.json
#	site2/website-next/versions.json
@lhotari lhotari mentioned this pull request Dec 17, 2021
Open
@sijie sijie mentioned this pull request Dec 17, 2021
Open
@lhotari lhotari mentioned this pull request Dec 18, 2021
Merged
1 task
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
@lhotari
Fix environment variable assignment in startup scripts (apache#13025)
60fcb07 
- don't combine variables in BOOKIE_EXTRA_OPTS
- don't add PULSAR_GC_LOG to PULSAR_EXTRA_OPTS
- make BOOKIE_GC_LOG default to PULSAR_GC_LOG for consistency
- apply PULSAR_EXTRA_OPTS and BOOKIE_EXTRA_OPTS after other settings so that it is possible to override
  settings defined by PULSAR_MEM / PULSAR_GC
@dave2wave
Copy link
Member

Please make sure this change is referenced in the 2.10 release notes as it is a breaking change for OpenMessaging/Benchmarks (at least for me)

gaozhangmin pushed a commit to gaozhangmin/pulsar that referenced this pull request May 6, 2022
@lhotari
Fix environment variable assignment in startup scripts (apache#13025)
93415be 
- don't combine variables in BOOKIE_EXTRA_OPTS
- don't add PULSAR_GC_LOG to PULSAR_EXTRA_OPTS
- make BOOKIE_GC_LOG default to PULSAR_GC_LOG for consistency
- apply PULSAR_EXTRA_OPTS and BOOKIE_EXTRA_OPTS after other settings so that it is possible to override
  settings defined by PULSAR_MEM / PULSAR_GC

(cherry picked from commit f00ee7b)
@horizonzy horizonzy mentioned this pull request Feb 7, 2023
Merged
15 tasks
@tisonkun tisonkun mentioned this pull request Mar 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MMirelli MMirelli MMirelli left review comments

@eolivelli eolivelli eolivelli approved these changes

@315157973 315157973 315157973 approved these changes

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@BewareMyPower BewareMyPower Awaiting requested review from BewareMyPower

@zymap zymap Awaiting requested review from zymap

Assignees

@lhotari lhotari

Labels
doc-not-needed Your PR changes do not impact docs type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lhotari @dave2wave @eolivelli @315157973 @MMirelli @nicoloboschi