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

[ISSUE #5484] Replace Logging Module with Shaded Logback #5524

Merged
merged 21 commits into from
Nov 16, 2022
Merged

[ISSUE #5484] Replace Logging Module with Shaded Logback #5524

merged 21 commits into from
Nov 16, 2022

Conversation

aaron-ai
Copy link
Member

Fixes #5484

@aaron-ai aaron-ai marked this pull request as draft November 14, 2022 07:17
@aaron-ai aaron-ai force-pushed the remove_logging_module branch 4 times, most recently from ee8f2cd to c471b61 Compare November 14, 2022 09:11
@lizhanhui lizhanhui self-requested a review November 15, 2022 03:31
@lizhanhui lizhanhui marked this pull request as ready for review November 15, 2022 11:34
@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2022 •
edited

Codecov Report

Merging #5524 (1bbf9f3) into develop (b39f687) will decrease coverage by 0.23%.
The diff coverage is 76.73%.

@@              Coverage Diff              @@
##             develop    #5524      +/-   ##
=============================================
- Coverage      42.65%   42.41%   -0.24%     
+ Complexity      8013     7933      -80     
=============================================
  Files           1031     1020      -11     
  Lines          72735    71142    -1593     
  Branches        9610     9380     -230     
=============================================
- Hits           31022    30172     -850     
+ Misses         37772    37153     -619     
+ Partials        3941     3817     -124
Impacted Files Coverage Δ
...apache/rocketmq/broker/BrokerPreOnlineService.java 0.00% <0.00%> (ø)
...java/org/apache/rocketmq/broker/BrokerStartup.java 8.00% <0.00%> (+0.25%) ⬆️
...ketmq/broker/dledger/DLedgerRoleChangeHandler.java 0.00% <0.00%> (ø)
...he/rocketmq/broker/filtersrv/FilterServerUtil.java 0.00% <ø> (ø)
.../broker/longpolling/LmqPullRequestHoldService.java 0.00% <0.00%> (ø)
...etmq/broker/processor/ForwardRequestProcessor.java 0.00% <0.00%> (ø)
...main/java/org/apache/rocketmq/client/MQHelper.java 0.00% <0.00%> (ø)
...client/consumer/MQPullConsumerScheduleService.java 0.00% <0.00%> (ø)
...sumer/rebalance/AllocateMessageQueueAveragely.java 86.66% <ø> (+7.71%) ⬆️
...balance/AllocateMessageQueueAveragelyByCircle.java 90.00% <ø> (+11.42%) ⬆️
... and 241 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lizhanhui
lizhanhui previously approved these changes Nov 16, 2022
View reviewed changes
@zhouxinyu
Copy link
Member

Consider replacing the current logging module with a simple new module containing a logback dependency and customized configurators. We could shade the lagback in the compile stage.

@lizhanhui
Copy link
Contributor

@zhouxinyu

Consider replacing the current logging module with a simple new module containing a logback dependency and customized configurators. We could shade the lagback in the compile stage.

Shading slf4j and logback is not of core concern and their release cycles match their own upstream vendors...I think it is best to externalize it to its own repository.

@lizhanhui lizhanhui merged commit 809ff6b into apache:develop Nov 16, 2022
@zhouxinyu
Copy link
Member

@zhouxinyu

Consider replacing the current logging module with a simple new module containing a logback dependency and customized configurators. We could shade the lagback in the compile stage.

Shading slf4j and logback is not of core concern and their release cycles match their own upstream vendors...I think it is best to externalize it to its own repository.

But we indeed do the shading work, hiding this work in another repo[rocketmq-logging] doesn't change this truth.

Furthermore, we have some customized configurators should belong to rocketmq repo.

I think there is no one who wants to maintain the new repo rocketmq-logging in aliyun-mq organization.

@zhouxinyu
Copy link
Member

@lizhanhui Let's revert this merge and try get some feedbacks from the author @aaron-ai

zhouxinyu added a commit that referenced this pull request Nov 17, 2022
@zhouxinyu
Revert "[ISSUE #5484] Replace Logging Module with Shaded Logback (#5524…
6f6195d 
…)"

This reverts commit 809ff6b.
@aaron-ai
Copy link
Member Author

@lizhanhui Let's revert this merge and try get some feedbacks from the author @aaron-ai

OK, I will refine the shading part.

aaron-ai pushed a commit that referenced this pull request Nov 17, 2022
@zhouxinyu
Revert "[ISSUE #5484] Replace Logging Module with Shaded Logback (#5524…
506b563 
…)" (#5537)

This reverts commit 809ff6b.
drpmma pushed a commit that referenced this pull request Feb 21, 2023
@aaron-ai @lizhanhui @drpmma
[ISSUE #5484] Replace Logging Module with Shaded Logback (#5524)
40dbc3a 
* Remove ClientLogger

* WIP

* WIP

* Rename logger to log

* Make it compile

* WIP

* Fix bazel to use shaded slf4j

* Fix DeleteExpiredCommitLogSubCommandTest

* Fix pom.xml duplication

* Fix maven deps

* Add test logback configuration file

* Fix unit test output

* Fix logback configuration file

* All logging are made on top of slf4j

* Fix test log configuration file name

* Fix test log configuration file name for test module

* All logging are shaded slf4j targeted

* DLedger has an explicit dependency on slf4j

* Fix DLedger

* Fix DLeader issue

* Move logback configuration files to each module

Co-authored-by: Li Zhanhui <lizhanhui@gmail.com>
drpmma pushed a commit that referenced this pull request Feb 21, 2023
@zhouxinyu @drpmma
Revert "[ISSUE #5484] Replace Logging Module with Shaded Logback (#5524…
ea948ea 
…)" (#5537)

This reverts commit 809ff6b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizhanhui lizhanhui lizhanhui approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RIP-56] Replace Logging Module with Shaded Logback
4 participants
@aaron-ai @codecov-commenter @zhouxinyu @lizhanhui