[GitHub] [kafka] C0urante commented on a diff in pull request #12490: KAFKA-14147: Prevent deferredTaskUpdates map from growing monotonically in KafkaConfigBackingStore

[GitBox]

C0urante commented on code in PR #12490:
URL: https://github.com/apache/kafka/pull/12490#discussion_r950226900


##
connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaConfigBackingStoreTest.java:
##
@@ -842,6 +842,7 @@ public void testBackgroundConnectorDeletion() throws 
Exception {
 // Task configs for the deleted connector should also be removed from 
the snapshot
 assertEquals(Collections.emptyList(), 
configState.allTaskConfigs(CONNECTOR_IDS.get(0)));
 assertEquals(0, configState.taskCount(CONNECTOR_IDS.get(0)));
+assertEquals(0, configStorage.deferredTaskUpdates.size());

Review Comment:
   Nit: it's better if we can do a comparison of the entire map here, since 
that provides better error messages if the assertion fails.
   
   Also, we should add a comment explaining why we have this check since it may 
get removed in a future change if it's unclear why this is necessary.
   
   ```suggestion
   // Make sure the deleted connector is removed from the map in order 
to prevent unbounded growth
   assertEquals(Collections.emptyMap(), 
configStorage.deferredTaskUpdates);
   ```



-- 
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

[GitHub] [kafka] C0urante commented on a diff in pull request #12490: KAFKA-14147: Prevent deferredTaskUpdates map from growing monotonically in KafkaConfigBackingStore

[GitBox]

C0urante commented on code in PR #12490:
URL: https://github.com/apache/kafka/pull/12490#discussion_r948191499


##
connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java:
##
@@ -853,6 +853,9 @@ private void processConnectorConfigRecord(String 
connectorName, SchemaAndValue v
 connectorConfigs.remove(connectorName);
 connectorTaskCounts.remove(connectorName);
 taskConfigs.keySet().removeIf(taskId -> 
taskId.connector().equals(connectorName));
+deferredTaskUpdates.remove(connectorName);
+connectorTaskCountRecords.remove(connectorName);

Review Comment:
   👍 happy to help!
   
   RE testing: I wouldn't overthink it; you can just upgrade the visibility of 
`deferredTaskUpdates` from private to package-private (possibly with a `// 
visible for testing` comment above the field declaration) and then probe its 
contents in the `KafkaConfigBackingStoreTest` suite, either within existing 
tests or with a new test.



-- 
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

[GitHub] [kafka] C0urante commented on a diff in pull request #12490: KAFKA-14147: Prevent deferredTaskUpdates map from growing monotonically in KafkaConfigBackingStore

[GitBox]

C0urante commented on code in PR #12490:
URL: https://github.com/apache/kafka/pull/12490#discussion_r941405027


##
connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java:
##
@@ -853,6 +853,9 @@ private void processConnectorConfigRecord(String 
connectorName, SchemaAndValue v
 connectorConfigs.remove(connectorName);
 connectorTaskCounts.remove(connectorName);
 taskConfigs.keySet().removeIf(taskId -> 
taskId.connector().equals(connectorName));
+deferredTaskUpdates.remove(connectorName);
+connectorTaskCountRecords.remove(connectorName);

Review Comment:
   > Tbh, it doesn't really seem like it's worth the mess of null handling 
everywhere. I'm gonna back out this change and make this a single line PR 😆
   
   A single line PR... with tests? 😄
   
   Your understanding is correct--we only track task generations in memory, 
different herders may have different generations for the same set of task 
configs, and we use generations to abort task startup after initializing their 
transactional producer and to abort persisting zombie fencing records to the 
config topic.
   
   The reason this is all fine is that we don't really need to track an exact 
generation number; all we have to do is track whether a new set of task configs 
for a given connector has appeared after a specific set of task configs. 
Compaction should not change the fact that, once we have a generation number 
for a set of task configs, generation numbers for later task configs will be 
greater than that number.



-- 
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