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

ZOOKEEPER-4465: zooinspector logback pattern config add escape for '(' and ')' #109

Conversation

rahulrane50
Copy link
Collaborator

Description

Cherry picked #PR1814

Tests

The following tests are written for this issue:
CI and end-to-end tests
Local zookeeper testing done.

Changes that Break Backward Compatibility (Optional)

NA

Documentation (Optional)

In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)

…' and ')'

zooinspect logback config file `logback.xml` currently use a pattern of this
`<pattern>%5p [%t] (%F:%L) - %m%n</pattern> `
which not escape the '(' and ')', cause logback to ignore parts after ')'.

according to logback documents, '(' and ')' is used for grouping, need escape by `\` if used as normal char
https://logback.qos.ch/manual/layouts.html#grouping

this pr update it to (add '\' to escape)
`<pattern>%5p [%t] \(%F:%L\) - %m%n</pattern> `

Author: 67 <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Andor Molnar <[email protected]>, Mate Szalay-Beko <[email protected]>

Closes apache#1814 from iamgd67/ZOOKEEPER-4465
@rahulrane50 rahulrane50 requested a review from mgao0 September 29, 2022 17:51
@rahulrane50
Copy link
Collaborator Author

@desaikomal

Copy link

@mgao0 mgao0 left a comment

Choose a reason for hiding this comment

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

LGTM.
Some side notes: I noticed that the mvn test has some failed tests, I think they are unrelated to this change though. Could you change the CI script so that the check fails when there are failed tests? Otherwise, we need to manually check every time and it makes the CI useless.

@rahulrane50
Copy link
Collaborator Author

Hey thanks for looking into logs and CI results. So actually way it works is it re-runs few of the flaky tests. So even if you are seeing few failures in logs eventually it did get pass successfully other CI will fail.
Tests run: 3158, Failures: 0, Errors: 0, Skipped: 3, Flakes: 3

@rahulrane50 rahulrane50 merged commit ce148a7 into linkedin:li-dev/log4j-to-logback Sep 30, 2022
abhilash1in pushed a commit that referenced this pull request Jun 16, 2023
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.

3 participants