xichen01 commented on PR #6656:
URL: https://github.com/apache/ozone/pull/6656#issuecomment-2102543957
> @xichen01 , thanks a lot for working on this! How about add a
`StringWithByteString` class as below? Than, it won't have any additional cache
lookups.
>
> ```java
> diff --git
a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
> index 26f6be44e5..d62af61d2a 100644
> ---
a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
> +++
b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
> @@ -801,7 +801,7 @@ public static Map<String, String>
processForLogging(OzoneConfiguration conf) {
> }
>
> @Nonnull
> - public static String threadNamePrefix(@Nullable String id) {
> + public static String threadNamePrefix(@Nullable Object id) {
> return id != null && !"".equals(id)
> ? id + "-"
> : "";
> diff --git
a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java
b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java
> index b455daba52..b00c3d623d 100644
> ---
a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java
> +++
b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java
> @@ -26,6 +26,7 @@
>
> import com.fasterxml.jackson.annotation.JsonIgnore;
> import com.google.common.collect.ImmutableSet;
> +import com.google.protobuf.ByteString;
> import org.apache.hadoop.hdds.DatanodeVersion;
> import org.apache.hadoop.hdds.HddsUtils;
> import org.apache.hadoop.hdds.annotation.InterfaceAudience;
> @@ -77,14 +78,33 @@ public static Codec<DatanodeDetails> getCodec() {
> return CODEC;
> }
>
> + static class StringWithByteString {
> + private final String string;
> + private final ByteString bytes;
> +
> + StringWithByteString(String string) {
> + this.string = string;
> + this.bytes = ByteString.copyFromUtf8(string);
> + }
> +
> + @Override
> + public String toString() {
> + return string;
> + }
> +
> + ByteString getBytes() {
> + return bytes;
> + }
> + }
> +
> /**
> * DataNode's unique identifier in the cluster.
> */
> private final UUID uuid;
> - private final String uuidString;
> + private final StringWithByteString uuidString;
> private final String threadNamePrefix;
>
> - private String ipAddress;
> + private StringWithByteString ipAddress;
> private String hostName;
> private final List<Port> ports;
> private String certSerialId;
> @@ -100,9 +120,9 @@ public static Codec<DatanodeDetails> getCodec() {
> private DatanodeDetails(Builder b) {
> super(b.hostName, b.networkLocation, NetConstants.NODE_COST_DEFAULT);
> uuid = b.id;
> - uuidString = uuid.toString();
> + uuidString = new StringWithByteString(uuid.toString());
> threadNamePrefix = HddsUtils.threadNamePrefix(uuidString);
> - ipAddress = b.ipAddress;
> + ipAddress = new StringWithByteString(b.ipAddress);
> hostName = b.hostName;
> ports = b.ports;
> certSerialId = b.certSerialId;
> @@ -127,7 +147,7 @@ public DatanodeDetails(DatanodeDetails
datanodeDetails) {
> datanodeDetails.getParent(), datanodeDetails.getLevel(),
> datanodeDetails.getCost());
> this.uuid = datanodeDetails.uuid;
> - this.uuidString = uuid.toString();
> + this.uuidString = new StringWithByteString(uuid.toString());
> threadNamePrefix = HddsUtils.threadNamePrefix(uuidString);
> this.ipAddress = datanodeDetails.ipAddress;
> this.hostName = datanodeDetails.hostName;
> @@ -161,7 +181,7 @@ public UUID getUuid() {
> * @return UUID of DataNode
> */
> public String getUuidString() {
> - return uuidString;
> + return uuidString.toString();
> }
>
> /**
> @@ -170,7 +190,7 @@ public String getUuidString() {
> * @param ip IP Address
> */
> public void setIpAddress(String ip) {
> - this.ipAddress = ip;
> + this.ipAddress = new StringWithByteString(ip);
> }
>
> /**
> @@ -179,7 +199,7 @@ public void setIpAddress(String ip) {
> * @return IP address
> */
> public String getIpAddress() {
> - return ipAddress;
> + return ipAddress.toString();
> }
>
> /**
> @@ -429,7 +449,7 @@ public HddsProtos.DatanodeDetailsProto.Builder
toProtoBuilder(
> builder.setUuid(getUuidString());
>
> if (ipAddress != null) {
> - builder.setIpAddress(ipAddress);
> + builder.setIpAddressBytes(ipAddress.getBytes());
> }
> if (hostName != null) {
> builder.setHostName(hostName);
> ```
Thanks for the suggestion, but I noticed that in OM, there are quite a few
instances of `DatanodeDetail`, and this implementation adds more memory
consumption to the member variable than before.
The current my PR idea is not to add extra memory consumption, so it uses a
global static Map to cache the `ByteString`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
