showuon commented on code in PR #11983:
URL: https://github.com/apache/kafka/pull/11983#discussion_r861528608
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeConnectProtocol.java:
##########
@@ -230,15 +230,16 @@ public static ExtendedWorkerState
deserializeMetadata(ByteBuffer buffer) {
* ScheduledDelay => Int32
* </pre>
*/
- public static ByteBuffer serializeAssignment(ExtendedAssignment
assignment) {
+ public static ByteBuffer serializeAssignment(ExtendedAssignment
assignment, boolean sessioned) {
// comparison depends on reference equality for now
if (assignment == null ||
ExtendedAssignment.empty().equals(assignment)) {
return null;
}
Struct struct = assignment.toStruct();
- ByteBuffer buffer =
ByteBuffer.allocate(CONNECT_PROTOCOL_HEADER_V1.sizeOf()
+ Struct protocolHeader = sessioned ? CONNECT_PROTOCOL_HEADER_V2 :
CONNECT_PROTOCOL_HEADER_V1;
+ ByteBuffer buffer = ByteBuffer.allocate(protocolHeader.sizeOf()
+
ASSIGNMENT_V1.sizeOf(struct));
- CONNECT_PROTOCOL_HEADER_V1.writeTo(buffer);
+ protocolHeader.writeTo(buffer);
Review Comment:
Nice catch! But I wonder why can't we get the protocol version from
`ExtendedAssignment.version()`?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignor.java:
##########
@@ -336,13 +332,16 @@ ClusterAssignment performTaskAssignment(
log.debug("Incremental connector assignments: {}",
incrementalConnectorAssignments);
log.debug("Incremental task assignments: {}",
incrementalTaskAssignments);
+ Map<String, Collection<String>> revokedConnectors =
transformValues(toRevoke, ConnectorsAndTasks::connectors);
+ Map<String, Collection<ConnectorTaskId>> revokedTasks =
transformValues(toRevoke, ConnectorsAndTasks::tasks);
+
return new ClusterAssignment(
incrementalConnectorAssignments,
incrementalTaskAssignments,
- transformValues(toRevoke, ConnectorsAndTasks::connectors),
- transformValues(toRevoke, ConnectorsAndTasks::tasks),
- connectorAssignments,
- taskAssignments
+ revokedConnectors,
+ revokedTasks,
+ diff(connectorAssignments, revokedConnectors),
+ diff(taskAssignments, revokedTasks)
Review Comment:
Why we returned `diff(connectorAssignments, revokedConnectors)` now? Was it
wrong before?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignor.java:
##########
@@ -108,18 +107,15 @@ public Map<String, ByteBuffer> performAssignment(String
leaderId, String protoco
log.debug("Max config offset root: {}, local snapshot config offsets
root: {}",
maxOffset, coordinator.configSnapshot().offset());
- short protocolVersion = memberConfigs.values().stream()
- .allMatch(state -> state.assignment().version() ==
CONNECT_PROTOCOL_V2)
- ? CONNECT_PROTOCOL_V2
- : CONNECT_PROTOCOL_V1;
+ short protocolVersion =
ConnectProtocolCompatibility.fromProtocol(protocol).protocolVersion();
Review Comment:
I think this change makes sense because the `protocol` is decided by group
coordinator, which is selected by one protocol supported by all clients. So
deciding the protocol version by `protocol` is fine. Thanks.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
