sanpwc commented on code in PR #3629:
URL: https://github.com/apache/ignite-3/pull/3629#discussion_r1576010330
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -191,16 +191,18 @@ public CompletableFuture<Void> start() {
updateLog.registerUpdateHandler(new OnUpdateHandlerImpl());
- updateLog.start();
+ updateLog.startAsync();
Review Comment:
That's not part of your PR actually, but anyway, we should not ignore
`updateLog.startAsync();` future. Proper async postfix 've made it visible.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -191,16 +191,18 @@ public CompletableFuture<Void> start() {
updateLog.registerUpdateHandler(new OnUpdateHandlerImpl());
- updateLog.start();
+ updateLog.startAsync();
return nullCompletedFuture();
}
@Override
- public void stop() throws Exception {
+ public CompletableFuture<Void> stopAsync() {
busyLock.block();
versionTracker.close();
- updateLog.stop();
+ updateLog.stopAsync();
Review Comment:
Basically same as above, that doesn't look right. Why updateLog.stopAsync()
future is ignored?
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -239,9 +239,9 @@ public void testNoInteractionsAfterStop() throws Exception {
CompletableFuture<Void> readyFuture = manager.catalogReadyFuture(1);
assertFalse(readyFuture.isDone());
- manager.stop();
+ manager.stopAsync();
Review Comment:
Here and in other places, let's add verification that corresponding
startAsync/stopAsync futures were completed successfully. I'm talking about
`assertThat(manager.stopAsync(), willCompleteSuccessfully());` In some places
we may combine components start/stop futures and assert that combined one was
completed successfully - not mandatory action though.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
