KAFKA-3459: Returning zero task configurations from a connector does not properly clean up existing tasks#1248
KAFKA-3459: Returning zero task configurations from a connector does not properly clean up existing tasks#1248Ishiihara wants to merge 9 commits into
Conversation
|
@Ishiihara How much trouble would it be to add a unit test? |
|
@hachikuji Unit test added. Please take another look. |
There was a problem hiding this comment.
Minor: maybe move this to initialization?
int newTaskCount = configs.size();
|
@hachikuji Updated to |
| * | ||
| * @param configs map containing task configurations | ||
| * @param connector the connector to write task configuration | ||
| * @param configs map containing task configurations for the connector |
There was a problem hiding this comment.
Nitpick: this parameter is no longer a map
| } | ||
|
|
||
| private static Map<ConnectorTaskId, Map<String, String>> taskConfigListAsMap(String connName, List<Map<String, String>> configs) { | ||
| private static Map<Integer, Map<String, String>> taskConfigListAsMap(List<Map<String, String>> configs) { |
There was a problem hiding this comment.
Is this method used anymore?
There was a problem hiding this comment.
Good catch and ack.
|
@Ishiihara Left a couple minor comments, but mostly LGTM. Thanks for trying to push through the refactor I suggested. It looks like it worked out well for |
| // update of all tasks that are expected based on the number of tasks in the commit message. | ||
| Map<String, Set<Integer>> updatedConfigIdsByConnector = taskIdsByConnector(deferred); | ||
| Set<Integer> taskIdSet = updatedConfigIdsByConnector.get(connectorName); | ||
| Set<Integer> taskIdSet = taskIds(deferred); |
There was a problem hiding this comment.
This can no longer return null based on the current implementation, so the subsequent check no longer makes sense. Although it looks like it was already impossible with the taskIdsByConnector code. I can't remember the exact context of KAFKA-3321 (I know we added this to help @gwenshap debug a transient system test failure). We might want to figure out if we can get rid of the check for a null taskIdSet now.
There was a problem hiding this comment.
I will follow up with @gwenshap to see whether we can remove or not.
There was a problem hiding this comment.
If it can't return null, what's the harm in checking?
completeTaskIdSet will still NPE if taskIdSet is null, so I think that being sure is very important.
|
@Ishiihara Same as @hachikuji, few minor comments but mostly LGTM. |
…onfigs from other connectors
|
@ewencp Addressed review comments. PTAL. |
|
LGTM, thanks @Ishiihara! |
…not properly clean up existing tasks hachikuji ewencp Can you take a look when you have time? Author: Liquan Pei <liquanpei@gmail.com> Reviewers: Jason Gustafson <jason@confluent.io>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#1248 from Ishiihara/kafka-3459
4 participants
@hachikuji @ewencp Can you take a look when you have time?