-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[improve][build]enable FindPublicAttributes check of SpotBugs #24285
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
Conversation
lhotari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, @3pacccccc . I think that this is the correct way how to enable FindPublicAttributes check. For some reason, public fields have been used in Pulsar code base extensively, perhaps due to the coding style and possibly also due to a belief that setters would be causing addition overhead. Modern JVMs can eliminate the overhead, so it's not really useful to avoid using setters.
The main challenge in this change is to ensure that we don't break current public interfaces or internal interfaces that are are used when there's a mixed version Pulsar cluster running during a new version upgrade or downgrade. Public interfaces extend also to Pulsar plugins and they aren't very well defined.
Since there's at least one breaking change in the public API at the moment, it's worth starting a discussion on the dev mailing list about these changes and the FindPublicAttributes check of SpotBugs. It's usually necessary to write a PIP so that everyone becomes aware of this change and that we properly document the considerations and make a decision about it. It's also possible that we decide to keep on ignoring the FindPublicAttributes check in SpotBugs, but it's useful to make that decision as part of handling the PIP. That's why the PIP should be mainly about deciding whether to handle it or not and when would we possibly ignore it.
| @Getter | ||
| @Setter | ||
| private long storageSize; | ||
|
|
||
| /** Total number of messages. */ | ||
| public long totalMessages; | ||
| @Getter | ||
| @Setter | ||
| private long totalMessages; | ||
|
|
||
| /** Total backlog. */ | ||
| public long messageBacklog; | ||
| @Getter | ||
| @Setter | ||
| private long messageBacklog; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are most likely a breaking change in the public API. In other stats classes there's a separate "Impl" class and a separate interface. I don't see a way how to preserve API compatibility and perhaps we should just break the API when making the change.
| private long messageBacklog; | ||
|
|
||
| /** Broker host where this stat was generated. */ | ||
| public final String brokerName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These final fields should be handled with getters for consistency.
Fixes #24245
Motivation
enable FindPublicAttributes check of SpotBugs
Modifications
as the FindPublicAttributes check of SpotBugs requires, change the Primitive field from public into private to limit external accessibility.
Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: 3pacccccc#6