adoroszlai commented on code in PR #5417:
URL: https://github.com/apache/ozone/pull/5417#discussion_r1948161249
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -504,20 +513,21 @@ public HddsProtos.DatanodeDetailsProto toProto(int
clientVersion, Set<Port.Name>
* If empty, all available ports will be included.
* @return A {@link HddsProtos.DatanodeDetailsProto.Builder} Object.
*/
-
+ @VisibleForTesting
public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder(
int clientVersion, Set<Port.Name> filterPorts) {
- HddsProtos.UUID uuid128 = HddsProtos.UUID.newBuilder()
- .setMostSigBits(uuid.getMostSignificantBits())
- .setLeastSigBits(uuid.getLeastSignificantBits())
- .build();
+
+ HddsProtos.UUID uuid128 = id.toProto().getUuid();
HddsProtos.DatanodeDetailsProto.Builder builder =
- HddsProtos.DatanodeDetailsProto.newBuilder()
- .setUuid128(uuid128);
+ HddsProtos.DatanodeDetailsProto.newBuilder();
- builder.setUuidBytes(uuidString.getBytes());
+ builder.setId(id.toProto());
Review Comment:
`id.toProto()` does not need to be called twice.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -619,11 +629,11 @@ public void setCurrentVersion(int currentVersion) {
@Override
public String toString() {
- return uuidString + "(" + hostName + "/" + ipAddress + ")";
+ return id + "(" + hostName + "/" + ipAddress + ")";
}
public String toDebugString() {
- return uuid.toString() + "{" +
+ return id.toString() + "{" +
Review Comment:
nit: can drop explicit `toString()`
```suggestion
return id + "{" +
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -504,20 +513,21 @@ public HddsProtos.DatanodeDetailsProto toProto(int
clientVersion, Set<Port.Name>
* If empty, all available ports will be included.
* @return A {@link HddsProtos.DatanodeDetailsProto.Builder} Object.
*/
-
+ @VisibleForTesting
public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder(
int clientVersion, Set<Port.Name> filterPorts) {
Review Comment:
nit: I would like to omit `@VisibleForTesting`. It tends to get outdated
(it's easy to add a call in prod code and keep the annotation), and
`toProtoBuilder` is safe to call for any code.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -504,20 +513,21 @@ public HddsProtos.DatanodeDetailsProto toProto(int
clientVersion, Set<Port.Name>
* If empty, all available ports will be included.
* @return A {@link HddsProtos.DatanodeDetailsProto.Builder} Object.
*/
-
+ @VisibleForTesting
public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder(
int clientVersion, Set<Port.Name> filterPorts) {
- HddsProtos.UUID uuid128 = HddsProtos.UUID.newBuilder()
- .setMostSigBits(uuid.getMostSignificantBits())
- .setLeastSigBits(uuid.getLeastSignificantBits())
- .build();
+
+ HddsProtos.UUID uuid128 = id.toProto().getUuid();
HddsProtos.DatanodeDetailsProto.Builder builder =
- HddsProtos.DatanodeDetailsProto.newBuilder()
- .setUuid128(uuid128);
+ HddsProtos.DatanodeDetailsProto.newBuilder();
- builder.setUuidBytes(uuidString.getBytes());
+ builder.setId(id.toProto());
+ // Both are deprecated.
+ builder.setUuid128(uuid128);
+ builder.setUuid(new UUID(uuid128.getMostSigBits(),
uuid128.getLeastSigBits())
+ .toString());
Review Comment:
Can we use `id.toString()` here?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -619,11 +629,11 @@ public void setCurrentVersion(int currentVersion) {
@Override
public String toString() {
- return uuidString + "(" + hostName + "/" + ipAddress + ")";
+ return id + "(" + hostName + "/" + ipAddress + ")";
Review Comment:
Can we store `uuidString` in `DatanodeID` and return it in `toString`, to
avoid repeated conversion?
--
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]
