Skip to content

HDDS-10745. Do not use BitSet for OzoneAcl.aclBitSet.#6581

Merged
szetszwo merged 7 commits intoapache:masterfrom
szetszwo:HDDS-10745
Apr 25, 2024
Merged

HDDS-10745. Do not use BitSet for OzoneAcl.aclBitSet.#6581
szetszwo merged 7 commits intoapache:masterfrom
szetszwo:HDDS-10745

Conversation

@szetszwo
Copy link
Contributor

What changes were proposed in this pull request?

The BitSet class used by aclBitSet is inefficient. We have to copy it in order to make aclBitSet immutable. BitSet is also inefficient in memory usage.

What is the link to the Apache JIRA

HDDS-10745

How was this patch tested?

Existing unit tests.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @szetszwo for the patch.

Comment on lines +300 to +301
public List<String> getAclStringList() {
return getAclList(aclBits, ACLType::name);
Copy link
Contributor

Choose a reason for hiding this comment

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

To fix test failure:

Suggested change
public List<String> getAclStringList() {
return getAclList(aclBits, ACLType::name);
@JsonIgnore
public List<String> getAclStringList() {
return getAclList(aclBits, ACLType::name);

Comment on lines 124 to 130
public static BitSet getACLBitSet(byte[] acls) {
return BitSet.valueOf(acls);
}

public static String getACLString(byte[] acls) {
return getACLString(BitSet.valueOf(acls));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest moving these to OzoneAcl and making them private. This would make it easier to completely get rid of BitSet in the future by reducing the public interface.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @szetszwo for updating the patch.

The new Proto2Utils class could be useful for other sub-modules, so I suggest putting it in hadoop-hdds/common instead of hadoop-ozone/common.

}

private Proto2Utils() { }

Copy link
Contributor

Choose a reason for hiding this comment

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

} is missing from EOF.

@szetszwo
Copy link
Contributor Author

@adoroszlai , thanks for reviewing this! That's a good suggestion. Let move it to hadoop-hdds/common.

@adoroszlai
Copy link
Contributor

Looks like failure in TestOzoneDebugShell is related:

2024-04-25 15:58:50,480 [DBScanner-0] ERROR debug.DBScanner (DBScanner.java:call(516)) - Exception parse Object
com.fasterxml.jackson.databind.JsonMappingException: Document nesting depth (1001) exceeds the maximum allowed (1000, from `StreamWriteConstraints.getMaxNestingDepth()`) (through reference chain: java.util.ArrayList[0]->org.apache.hadoop.ozone.OzoneAcl["toStringMethod"]->org.apache.ratis.util.MemoizedSupplier["initializer"]->org.apache.hadoop.ozone.OzoneAcl$$Lambda$832/0x00007f47105fdd68["arg$1"]->org.apache.hadoop.ozone.OzoneAcl["toStringMethod"]-...)
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:402)
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:361)
	at com.fasterxml.jackson.databind.ser.std.StdSerializer.wrapAndThrow(StdSerializer.java:323)
	at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:778)
	at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:183)

I think we need @JsonIgnore for the two new supplier fields:

private final Supplier<String> toStringMethod;
private final Supplier<Integer> hashCodeMethod;

@szetszwo szetszwo merged commit 383c83f into apache:master Apr 25, 2024
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 29, 2024
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.

2 participants