Skip to content

Conversation

@adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Fix findbugs warnings by making BucketArgs immutable. See #6193 for similar change in VolumeArgs.

M V EI: org.apache.hadoop.ozone.client.BucketArgs.getAcls() may expose internal representation by returning BucketArgs.acls  At BucketArgs.java:[line 130]
M V EI: org.apache.hadoop.ozone.client.BucketArgs.getMetadata() may expose internal representation by returning BucketArgs.metadata  At BucketArgs.java:[line 139]
M V EI2: org.apache.hadoop.ozone.client.BucketArgs$Builder.setAcls(List) may expose internal representation by storing an externally mutable object into BucketArgs$Builder.acls  At BucketArgs.java:[line 239]

Also eliminate @SuppressWarnings("parameternumber") by passing the builder to the constructor, instead of individual properties.

Create new constructor in OzoneAcl to simplify instantiation with multiple ACLType (avoid the need to mess with BitSet).

https://issues.apache.org/jira/browse/HDDS-10325

How was this patch tested?

$ hadoop-ozone/dev-support/checks/findbugs.sh -Dspotbugs.version=4.8.3.0
$ grep -c 'client.BucketArgs' target/findbugs/summary.txt
0

CI:
https://github.com/adoroszlai/ozone/actions/runs/7843743627

@adoroszlai adoroszlai added the code-cleanup Changes that aim to make code better, without changing functionality. label Feb 9, 2024
@adoroszlai adoroszlai self-assigned this Feb 9, 2024

private static BitSet bitSetOf(ACLType... acls) {
BitSet bits = new BitSet();
if (acls != null && acls.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (acls != null && acls.length > 0) {
if (acls != null) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the problem with acls.length > 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

The length > 0 is handled implicitly by the for loop, so its not really needed. I guess this is a nit - change it if there are other comments, otherwise it will do no harm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that it's not strictly necessary. I have two problems:

  1. Don't know the runtime cost of creating an iterator for the empty array in the for loop behind the scene. Maybe the compiler optimizes it away.
  2. Running full CI again for such a cosmetic change does not seem worth the effort. Especially when the reviewer cannot spend the extra 10 seconds to write "nit:" or something like that.

Copy link
Contributor

@sodonnel sodonnel left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kerneltime kerneltime left a comment

Choose a reason for hiding this comment

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

Just one minor nit

@adoroszlai adoroszlai merged commit 45c853c into apache:master Feb 12, 2024
@adoroszlai
Copy link
Contributor Author

Thanks @kerneltime, @sodonnel for the review.

@adoroszlai adoroszlai deleted the HDDS-10325 branch February 12, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-cleanup Changes that aim to make code better, without changing functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants