kirktrue commented on code in PR #21484:
URL: https://github.com/apache/kafka/pull/21484#discussion_r2819801604
##########
clients/src/main/java/org/apache/kafka/clients/admin/Admin.java:
##########
@@ -1229,7 +1229,7 @@ default ListPartitionReassignmentsResult
listPartitionReassignments(
}
/**
- * List all of the current partition reassignments
+ * List all the current partition reassignments
Review Comment:
```suggestion
* List all the current partition reassignments.
```
##########
clients/src/main/java/org/apache/kafka/clients/admin/AlterConfigOp.java:
##########
@@ -76,10 +79,18 @@ public enum OpType {
this.id = id;
}
+ /**
+ * Returns the byte ID associated with this operation type. This ID is
used for serialization
+ * and protocol communication.
+ */
public byte id() {
return id;
}
+ /**
+ * Returns the {@code OpType} corresponding to the given byte ID, or
{@code null} if the
+ * ID does not match any known operation type.
Review Comment:
Interesting that it returns `null` as opposed to throwing an
`IllegalArgumentException` like many of the other string/integer -> enum
methods do 🤔
##########
clients/src/main/java/org/apache/kafka/clients/admin/Admin.java:
##########
@@ -1250,6 +1250,8 @@ default ListPartitionReassignmentsResult
listPartitionReassignments(ListPartitio
}
/**
+ * List all the current partition reassignments
Review Comment:
Ditto as above.
##########
clients/src/main/java/org/apache/kafka/clients/admin/ClassicGroupDescription.java:
##########
@@ -91,23 +122,21 @@ public String groupId() {
}
/**
- * The group protocol type.
+ * The group protocol type, or the empty string.
Review Comment:
It doesn't look like there's anything preventing a `null` from being passed
to the constructor for `protocol`. `protocolData` is forced to an empty string,
but `protocol` is not 🤔
##########
clients/src/main/java/org/apache/kafka/clients/admin/ClassicGroupDescription.java:
##########
@@ -91,23 +122,21 @@ public String groupId() {
}
/**
- * The group protocol type.
+ * The group protocol type, or the empty string.
*/
public String protocol() {
return protocol;
}
/**
- * The group protocol data. The meaning depends on the group protocol type.
- * For a classic consumer group, this is the partition assignor name.
- * For a classic connect group, this indicates which Connect protocols are
enabled.
+ * The group protocol data.
Review Comment:
Is the detail in the previous description for this method incorrect now?
##########
clients/src/main/java/org/apache/kafka/clients/admin/AlterConfigOp.java:
##########
@@ -76,10 +79,18 @@ public enum OpType {
this.id = id;
}
+ /**
+ * Returns the byte ID associated with this operation type. This ID is
used for serialization
Review Comment:
We should be consistent, if possible. Either choose `id` or `ID`, preferably
the latter.
--
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]
