MINOR: Remove onPartitionsDeleted from GroupCoordinator interface #21263
Conversation
This patch removes the `onPartitionsDeleted` method from the `GroupCoordinator` interface by moving its functionality into `onMetadataUpdate`. The `MetadataDelta` already contains the deleted topic information via `delta.topicsDelta().deletedTopicIds()`, making the separate method redundant.
| // Wait on the results. | ||
| CompletableFuture.allOf(futures.toArray(new CompletableFuture<?>[0])); |
There was a problem hiding this comment.
There is a but here as the code does not actually waits on the future to complete.
squah-confluent
left a comment
squah-confluent
left a comment
There was a problem hiding this comment.
Thanks for the patch!
Not introduced in this PR but I'm not too happy about the onMetadataUpdate being the only method that throws ExecutionException and InteruptedException in the interface. Based on https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Future.html#get--, ExecutionException should never happen since we catch exceptions coming from the futures and maybe we could swallow InterruptedException and convert it to Thread.currentThread().interrupt()?
| } | ||
|
|
||
| @Test | ||
| public void testOnPartitionsDeletedWhenServiceIsNotStarted() { |
There was a problem hiding this comment.
Do we have an equivalent test for onMetadataUpdate? I couldn't find one.
Interesting... An alternative would be to use |
That would work nicely, we could use that instead. |
| case t: Throwable => metadataPublishingFaultHandler.handleFault("Error updating group " + | ||
| s"coordinator with deleted partitions in $deltaName", t) |
There was a problem hiding this comment.
This seems to be replaced by the general with local changes but I guess it's fine as we also log error message in futures exception
dongnuo123
left a comment
dongnuo123
left a comment
There was a problem hiding this comment.
Thanks for the PR! Lgtm.
|
|
||
| @Test | ||
| public void testCompleteTransactionWhenNotCoordinatorServiceStarted() { | ||
| public void testCompleteTransactionWhenNotCoordinatorServiceStarted() throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
Thanks for updating the PR. Did we miss removing these?
|
|
||
| @Test | ||
| public void testPersisterInitializeSuccess() { | ||
| public void testPersisterInitializeSuccess() throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
We missed removing these down here too
lianetm
left a comment
lianetm
left a comment
There was a problem hiding this comment.
Thanks! Just a comment on test
| // Verify method is blocked. | ||
| assertFalse(resultFuture.isDone()); |
There was a problem hiding this comment.
Is this enough to be sure that we're blocking? I guess the future here could be incomplete only because the async execution of onMetadataUpdate hasn't made it til the end where it's supposed to block/join on the write operations (not because it is blocked waiting on the operations completion). If so, this wouldn't catch a bug like the one you pointed out above with the missing join().
What about we verify that we did schedule the operations before this assert?
~ verify(runtime, timeout(xxx).times(2)).scheduleWriteAllOperation) + assertFalse(resultFuture.isDone())
There was a problem hiding this comment.
Good callout. Addressed it.
4 participants
This patch removes the
onPartitionsDeletedmethod from theGroupCoordinatorinterface by moving its functionality intoonMetadataUpdate. TheMetadataDeltaalready contains the deletedtopic information via
delta.topicsDelta().deletedTopicIds(), makingthe separate method redundant.
Reviewers: Dongnuo Lyu [email protected], Sean Quah
[email protected], Lianet Magrans [email protected]