Skip to content

[fix][broker] Log pulsar service config except sensitive info at startup#18179

Merged
AnonHxy merged 5 commits into
apache:masterfrom
zhaohaidao:feat/log-conf-except-sensitive-info
Oct 28, 2022
Merged

[fix][broker] Log pulsar service config except sensitive info at startup#18179
AnonHxy merged 5 commits into
apache:masterfrom
zhaohaidao:feat/log-conf-except-sensitive-info

Conversation

@zhaohaidao

@zhaohaidao zhaohaidao commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

Motivation

Printing service configuration at startup can help us effectively troubleshoot problems, but after #14421, the configuration at startup is no longer displayed.
According to the analysis, #14421 is mainly to avoid printing private information, such as passwords, rather than the entire configuration

Modifications

Log pulsar service config except sensitive info at startup

Verifying this change

  • Make sure that the change passes the CI checks.
  • This change is a trivial rework / code cleanup without any test coverage.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions

Copy link
Copy Markdown

@zhaohaidao Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Oct 25, 2022
@zhaohaidao zhaohaidao changed the title Log pulsar service config except sensitive info at startup [fix]Log pulsar service config except sensitive info at startup Oct 25, 2022
@zhaohaidao

Copy link
Copy Markdown
Contributor Author

rerun failure checks

@zhaohaidao zhaohaidao force-pushed the feat/log-conf-except-sensitive-info branch from b8913c8 to b3dc795 Compare October 25, 2022 02:00
@zhaohaidao zhaohaidao changed the title [fix]Log pulsar service config except sensitive info at startup [fix][broker]Log pulsar service config except sensitive info at startup Oct 25, 2022
@zhaohaidao zhaohaidao force-pushed the feat/log-conf-except-sensitive-info branch from b3dc795 to ac79fb8 Compare October 25, 2022 02:04
@zhaohaidao zhaohaidao changed the title [fix][broker]Log pulsar service config except sensitive info at startup [fix][broker] Log pulsar service config except sensitive info at startup Oct 25, 2022
)
private List<String> bootstrapNamespaces = new ArrayList<String>();

@ToString.Exclude

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder why this lombok anonotion can not take affect

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The current implementation uses ReflectionToStringBuilder to print the serialized result. If I understand correctly, ReflectionToStringBuilder.toString(config) does not recognize lombok's annotations, we need to use the corresponding annotations to filter privacy information, namely StringToExclude instead of lombok's String.ToExclude

... Alternatively the ToStringExclude annotation can be used to exclude fields from being incorporated in the result. ... public class ReflectionToStringBuilder extends ToStringBuilder { ...

@AnonHxy AnonHxy Oct 26, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your explain. now I see it. The root cause is that we didn't over write toString() method of ServiceConfiguration.

How about just change it like this:

@ToString
@Getter
@Setter
public class ServiceConfiguration

or

@Data
public class ServiceConfiguration

because it looks that we need the toString() method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your advice. I tried to add ToString annotation and it works as you expected.
I'm not sure if @DaTa is ok as it is equivalent to @Getter @Setter @requiredargsconstructor @tostring @EqualsAndHashCode.
If I understand correctly. We just need@Getter @Setter and @tostring only.

@zhaohaidao

Copy link
Copy Markdown
Contributor Author

rerun failure checks

@zhaohaidao

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@zhaohaidao

Copy link
Copy Markdown
Contributor Author

@AnonHxy @codelipenghui This pr passes all the tests in my repo. Please have a look if you have time. Thanks

@AnonHxy AnonHxy added the type/bug The PR fixed a bug or issue reported a bug label Oct 27, 2022
@AnonHxy AnonHxy added this to the 2.11.0 milestone Oct 27, 2022
@codecov-commenter

codecov-commenter commented Oct 27, 2022

Copy link
Copy Markdown

Codecov Report

Merging #18179 (228965c) into master (6c65ca0) will increase coverage by 4.00%.
The diff coverage is 35.26%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18179      +/-   ##
============================================
+ Coverage     34.91%   38.92%   +4.00%     
- Complexity     5707     8283    +2576     
============================================
  Files           607      683      +76     
  Lines         53396    67268   +13872     
  Branches       5712     7214    +1502     
============================================
+ Hits          18644    26183    +7539     
- Misses        32119    38072    +5953     
- Partials       2633     3013     +380     
Flag Coverage Δ
unittests 38.92% <35.26%> (+4.00%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...org/apache/bookkeeper/mledger/LedgerOffloader.java 0.00% <ø> (ø)
...che/bookkeeper/mledger/LedgerOffloaderFactory.java 0.00% <ø> (ø)
...pache/bookkeeper/mledger/LedgerOffloaderStats.java 0.00% <ø> (ø)
...ookkeeper/mledger/LedgerOffloaderStatsDisable.java 0.00% <ø> (ø)
...a/org/apache/bookkeeper/mledger/ManagedCursor.java 85.71% <ø> (ø)
...che/bookkeeper/mledger/ManagedLedgerException.java 41.17% <ø> (ø)
...bookkeeper/mledger/ManagedLedgerFactoryConfig.java 86.66% <ø> (ø)
...g/apache/bookkeeper/mledger/ManagedLedgerInfo.java 22.22% <ø> (ø)
...he/bookkeeper/mledger/OffloadedLedgerMetadata.java 0.00% <ø> (ø)
...ava/org/apache/bookkeeper/mledger/ScanOutcome.java 0.00% <ø> (ø)
... and 729 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants