You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "jojochuang (via GitHub)" <gi...@apache.org> on 2024/01/03 07:30:50 UTC
[PR] HDDS-10054. Reduce DataNode token verification heap allocation cost. [ozone]
jojochuang opened a new pull request, #5909:
URL: https://github.com/apache/ozone/pull/5909
## What changes were proposed in this pull request?
Remove unnecessary heap allocation associated with block token verification in DataNode.
Please describe your PR in detail:
Rewrite part of token verification code to use the underlying byte array directly. Avoid certain input/output stream objects that internally allocates sizable heap (4KB) each.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10054
## How was this patch tested?
Ran with freon hsync workload and verified the change does remove heap allocation using profiler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10054. Reduce DataNode token verification heap allocation cost. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5909:
URL: https://github.com/apache/ozone/pull/5909#discussion_r1440242131
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/token/ContainerTokenIdentifier.java:
##########
@@ -87,6 +87,16 @@ public void readFields(DataInput in) throws IOException {
this.containerID = ContainerID.getFromProtobuf(proto.getContainerId());
}
+ @Override
+ public void readFromByteArray(byte[] bytes) throws IOException {
+ ContainerTokenSecretProto proto =
+ ContainerTokenSecretProto.parseFrom(bytes);
+ setSecretKeyId(ProtobufUtils.fromProtobuf(proto.getSecretKeyId()));
+ setExpiry(Instant.ofEpochMilli(proto.getExpiryDate()));
+ setOwnerId(proto.getOwnerId());
+ this.containerID = ContainerID.getFromProtobuf(proto.getContainerId());
Review Comment:
Please extract this part to a method and reuse in `readFields`:
https://github.com/apache/ozone/blob/41cb6d0ea95aeddc59e98005529204278e9be02f/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/token/ContainerTokenIdentifier.java#L84-L87
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/token/OzoneBlockTokenIdentifier.java:
##########
@@ -141,6 +141,18 @@ public void readFields(DataInput in) throws IOException {
this.maxLength = token.getMaxLength();
}
+ @Override
+ public void readFromByteArray(byte[] bytes) throws IOException {
+ BlockTokenSecretProto token =
+ BlockTokenSecretProto.parseFrom(bytes);
+ setOwnerId(token.getOwnerId());
+ setExpiry(Instant.ofEpochMilli(token.getExpiryDate()));
+ setSecretKeyId(ProtobufUtils.fromProtobuf(token.getSecretKeyId()));
+ this.blockId = token.getBlockId();
+ this.modes = EnumSet.copyOf(token.getModesList());
+ this.maxLength = token.getMaxLength();
Review Comment:
Same here, please share with:
https://github.com/apache/ozone/blob/41cb6d0ea95aeddc59e98005529204278e9be02f/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/token/OzoneBlockTokenIdentifier.java#L136-L141
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10054. Reduce DataNode token verification heap allocation cost. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5909:
URL: https://github.com/apache/ozone/pull/5909#issuecomment-1881698946
@jojochuang please let me know if you are OK with me updating the PR with the minor refactoring I suggested
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10054. Reduce DataNode token verification heap allocation cost. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5909:
URL: https://github.com/apache/ozone/pull/5909#discussion_r1448731418
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/token/OzoneBlockTokenIdentifier.java:
##########
@@ -170,6 +181,21 @@ public void write(DataOutput out) throws IOException {
out.write(builder.build().toByteArray());
}
+ @Override
+ public byte[] getBytes() {
+ BlockTokenSecretProto.Builder builder = BlockTokenSecretProto.newBuilder()
+ .setBlockId(blockId)
+ .setOwnerId(getOwnerId())
+ .setSecretKeyId(ProtobufUtils.toProtobuf(getSecretKeyId()))
+ .setExpiryDate(getExpiryDate())
+ .setMaxLength(maxLength);
+ // Add access mode allowed
+ for (AccessModeProto mode : modes) {
+ builder.addModes(AccessModeProto.valueOf(mode.name()));
+ }
+ return builder.build().toByteArray();
+ }
Review Comment:
`getBytes()` is overridden (optimized) only for block token. Did you intend to do the same for container token?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10054. Reduce DataNode token verification heap allocation cost. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5909:
URL: https://github.com/apache/ozone/pull/5909
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10054. Reduce DataNode token verification heap allocation cost. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #5909:
URL: https://github.com/apache/ozone/pull/5909#discussion_r1449537677
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/token/OzoneBlockTokenIdentifier.java:
##########
@@ -170,6 +181,21 @@ public void write(DataOutput out) throws IOException {
out.write(builder.build().toByteArray());
}
+ @Override
+ public byte[] getBytes() {
+ BlockTokenSecretProto.Builder builder = BlockTokenSecretProto.newBuilder()
+ .setBlockId(blockId)
+ .setOwnerId(getOwnerId())
+ .setSecretKeyId(ProtobufUtils.toProtobuf(getSecretKeyId()))
+ .setExpiryDate(getExpiryDate())
+ .setMaxLength(maxLength);
+ // Add access mode allowed
+ for (AccessModeProto mode : modes) {
+ builder.addModes(AccessModeProto.valueOf(mode.name()));
+ }
+ return builder.build().toByteArray();
+ }
Review Comment:
Good idea.
Also refactored OzoneBlockTokenIdentifier write() to use getBytes().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org