-
Notifications
You must be signed in to change notification settings - Fork 14.5k
MINOR: Cleanup Tools Module (1/n) #20091
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
base: trunk
Are you sure you want to change the base?
Conversation
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 @sjhajharia for this patch, left some comments.
Set<AccessControlEntry> topicAcls = getAcl(opts, new HashSet<>(List.of(WRITE, DESCRIBE, CREATE))); | ||
Set<AccessControlEntry> transactionalIdAcls = getAcl(opts, new HashSet<>(List.of(WRITE, DESCRIBE))); |
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.
Could these properties be made immutable?
Set<AccessControlEntry> topicAcls = getAcl(opts, new HashSet<>(List.of(WRITE, DESCRIBE, CREATE))); | |
Set<AccessControlEntry> transactionalIdAcls = getAcl(opts, new HashSet<>(List.of(WRITE, DESCRIBE))); | |
Set<AccessControlEntry> topicAcls = getAcl(opts, Set.of(WRITE, DESCRIBE, CREATE)); | |
Set<AccessControlEntry> transactionalIdAcls = getAcl(opts, Set.of(WRITE, DESCRIBE)); |
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.
Just a reminder:
Set.of does not guarantee order, so it could make tests flaky or break public interfaces.
Please make sure that replacing it with Set.of() won’t cause any issues.
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.
The original code used new HashSet<>(List.of(...))
, so I believe the order was not guaranteed in the previous approach.
Now that Kafka support Java 17, this PR makes some changes in tools
module. The changes in this PR are limited to only some files. A future
PR(s) shall follow.
The changes mostly include:
Arrays.asList() are replaced with List.of()
with Map.of()
Sub modules targeted: tools/src/main