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.

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

[ROCKETMQ-292]Delete system.exit in MQAdminStartup #169

Closed
wants to merge 8 commits into from

Conversation

lindzh
Copy link
Contributor

@lindzh lindzh commented Sep 21, 2017

What is the purpose of the change

When using MQAdminStartup in a java process,some args problem may cause MQAdminStartup to call system.ext and this lead to main java process shutdown.
JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-278.

Brief change log

  • delete system.exit in mqadmin.

Verifying this change

  • This change is a trivial rework / code cleanup without unit test coverage.
  • execute mqadmin testa to verify.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ROCKETMQ-XXX] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand the pull request :
    • What is the purpose of the change
    • Brief change log
    • Verifying this change
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 38.826% when pulling a905b14 on lindzh:fix_main_system.exit into 27a678d on apache:develop.

@vongosling
Copy link
Member

Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.

Have you provide the test module ?

@vongosling
Copy link
Member

System.exit() can be used to run shutdown hooks before the program quits. This is a convenient way to handle shutdown in bigger programs, where all parts of the program can't (and shouldn't) be aware of each other.
Another, maybe more common, way to quit a program is to simply to reach the end of the main method. But if there are any non-daemon threads running, they will not be shut down and thus the JVM will not exit. Thus, if you have any such non-daemon threads, you need some other means (than the shutdown hooks) to shut down all non-daemon threads and release other resources. If there are no other non-daemon threads, returning from main will shut down the JVM and will call the shutdown hooks.

For this reason, I am glad to see this improvement, thanks @lindzh

@shroman @Jaskey any opinion about this :-)

@lindzh
Copy link
Contributor Author

lindzh commented Sep 29, 2017

I think there is no need to add any test case for this issue.Any opinion? IMO

@shroman
Copy link
Contributor

shroman commented Sep 29, 2017

I think we just need to check if there're no such non-daemon threads, and commit this pr ;)

@dongeforever
Copy link
Member

LGTM

1 similar comment
@Jaskey
Copy link
Contributor

Jaskey commented Oct 19, 2017

LGTM

Copy link
Member

@zhouxinyu zhouxinyu left a comment

Choose a reason for hiding this comment

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

LGTM~

zhouxinyu pushed a commit that referenced this pull request Dec 1, 2017
Author: lindzh <linsony0@163.com>

Closes #169 from lindzh/fix_main_system.exit.
@zhouxinyu
Copy link
Member

merged!

@zhouxinyu zhouxinyu closed this Dec 1, 2017
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
show accumulation
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
Issue apache#169

1. show accumulation
2. support to show selected topic

See merge request !24
JiaMingLiu93 pushed a commit to JiaMingLiu93/rocketmq that referenced this pull request May 28, 2020
Author: lindzh <linsony0@163.com>

Closes apache#169 from lindzh/fix_main_system.exit.
lollipopjin added a commit to lollipopjin/rocketmq that referenced this pull request Jun 16, 2022
…ature_1

* origin/develop_with_prehistory: (3800 commits)
  Issue apache#179 [BUG]check properties length
  Issue apache#161 printmsg bug 修复
  Issue apache#175 change version to 3.6.3-SNAPSHOT
  Issue apache#175 pub 3.6.2 server release
  Issue apache#174 顺序消息重试次数超过默认值,导致消息直接进入死信队列
  Issue apache#173 日志滚动异常,更新日志配置
  Issue apache#172 rebalance 抛 Error 异常,导致 rebalance 被中断
  Issue apache#171 指定消息 id 进行重新发送
  Issue apache#146 add trace hook switch and consume return type
  Issue apache#146 add trace hook switch and consume return type
  Issue apache#169
  Issue apache#158 1. Unify config manager to configuration in common. 2. Configuration is hold in controller. 3. Extend config object or property can register to configuration.
  Issue apache#163 1. command of get broker config
  Issue apache#163 1. Directly print msg after query by queue offset.
  Issue apache#163 1. format file header
  Issue apache#163 1. support for modifying the config of name server dynamically 2. add two request codes 3. add two commands 3. set config store path when start up using special property file
  Issue apache#137 【买卖家】队列热点数据分析
  Issue apache#162 ConsumerConnection 获取连接失败。
  Issue apache#161 printmsg 使用 tag 过滤时,拉消息不全
  Issue apache#160 重置位点优化
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants