Skip to content

Conversation

zhangjidi2016
Copy link
Contributor

Make sure set the target branch to develop

What is the purpose of the change

#3173

Brief changelog

XX

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github 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 [ISSUE #123] 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 what the pull request does, how, and why.
  • Write necessary unit-test(over 80% coverage) 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

coveralls commented Jul 23, 2021

Coverage Status

Coverage decreased (-1.4%) to 53.088% when pulling 3738214 on zhangjidi2016:broker_log into 143ac31 on apache:develop.

@zhangjidi2016
Copy link
Contributor Author

image

@zongtanghu zongtanghu modified the milestones: 4.9.1, 4.9.2 Jul 28, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2021

Codecov Report

Merging #3174 (fc5180e) into develop (1e982af) will increase coverage by 1.31%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3174      +/-   ##
=============================================
+ Coverage      47.84%   49.15%   +1.31%     
+ Complexity      4555     3672     -883     
=============================================
  Files            552      312     -240     
  Lines          36642    29380    -7262     
  Branches        4848     4183     -665     
=============================================
- Hits           17530    14441    -3089     
+ Misses         16883    12972    -3911     
+ Partials        2229     1967     -262     
Impacted Files Coverage Δ
...java/org/apache/rocketmq/broker/BrokerStartup.java 5.47% <0.00%> (-0.20%) ⬇️
...rocketmq/broker/pagecache/ManyMessageTransfer.java 36.36% <0.00%> (-9.80%) ⬇️
.../rocketmq/broker/pagecache/OneMessageTransfer.java 24.00% <0.00%> (-9.34%) ⬇️
...rg/apache/rocketmq/remoting/netty/NettyLogger.java 16.32% <0.00%> (-2.73%) ⬇️
...tmq/namesrv/processor/DefaultRequestProcessor.java 34.41% <0.00%> (-1.52%) ⬇️
...ocketmq/broker/processor/SendMessageProcessor.java 39.74% <0.00%> (-1.22%) ⬇️
...e/rocketmq/client/impl/consumer/RebalanceImpl.java 40.23% <0.00%> (-0.79%) ⬇️
...ion/AbstractTransactionalMessageCheckListener.java 75.67% <0.00%> (-0.65%) ⬇️
...he/rocketmq/remoting/protocol/RemotingCommand.java 78.22% <0.00%> (-0.41%) ⬇️
...org/apache/rocketmq/store/DefaultMessageStore.java 55.54% <0.00%> (-0.17%) ⬇️
... and 279 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e982af...fc5180e. Read the comment docs.

@RongtongJin
Copy link
Contributor

IMO, Could it be configured as optional? Otherwise, the sudden change of log directory is very confused for users.

Copy link
Contributor

@RongtongJin RongtongJin left a comment

Choose a reason for hiding this comment

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

IMO, Could it be configured as optional? Otherwise, the sudden change of log directory is very confused for users.

@zhangjidi2016
Copy link
Contributor Author

@RongtongJin @yuz10 can you review this pr again ?

@@ -187,6 +187,13 @@ public static BrokerController createBrokerController(String[] args) {
JoranConfigurator configurator = new JoranConfigurator();
configurator.setContext(lc);
lc.reset();
System.setProperty("brokerLogDir", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is set to "" here, will it be in the original directory by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, if brokerLogDir is "", the logPath in logback_broker.xml is default.

@francisoliverlee francisoliverlee modified the milestones: 4.9.2, 4.9.3 Oct 11, 2021
@yuz10 yuz10 requested a review from RongtongJin December 11, 2021 01:30
Copy link
Member

@francisoliverlee francisoliverlee left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -187,6 +187,8 @@

private boolean autoDeleteUnusedStats = false;

private boolean isolateLogEnable = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to add some comments to explain the meaning of this parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@francisoliverlee francisoliverlee merged commit ed5f4e4 into apache:develop Jan 7, 2022
@zhangjidi2016 zhangjidi2016 deleted the broker_log branch April 14, 2022 13:30
GenerousMan pushed a commit to GenerousMan/rocketmq that referenced this pull request Aug 12, 2022
[ISSUE apache#3173]Isolate Broker logs when multiple Broker services are deployed on the same machine.
carlvine500 pushed a commit to carlvine500/rocketmq-apache that referenced this pull request Sep 10, 2024
[ISSUE apache#3173]Isolate Broker logs when multiple Broker services are deployed on the same machine.
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.

Isolate Broker logs when multiple Broker services are deployed on the same machine
8 participants