rpuch commented on code in PR #4072: URL: https://github.com/apache/ignite-3/pull/4072#discussion_r1675394117
########## modules/index/src/main/java/org/apache/ignite/internal/index/ChangeIndexStatusTask.java: ########## @@ -333,8 +335,26 @@ private <T> CompletableFuture<T> inBusyLocks(Supplier<CompletableFuture<T>> supp try { return supplier.get(); + } catch (Throwable t) { Review Comment: We should not swallow `Error`s (which are not `AssertionError`). For them, we should probably log them and rethrow them after this. ########## modules/index/src/main/java/org/apache/ignite/internal/index/ChangeIndexStatusTask.java: ########## @@ -333,8 +335,26 @@ private <T> CompletableFuture<T> inBusyLocks(Supplier<CompletableFuture<T>> supp try { return supplier.get(); + } catch (Throwable t) { + return failedFuture(t); } finally { leaveBusy(); } } + + private MetaIndexStatusChange statusChange() { + IndexMeta indexMeta = indexMetaStorage.indexMeta(indexDescriptor.id()); + + if (indexMeta == null) { + // Index was destroyed under a low watermark, well, we need to build it. + throw new IndexTaskStoppingException(); + } + + MetaIndexStatusChange statusChange = indexMeta.statusChangeNullable(MetaIndexStatus.convert(indexDescriptor.status())); + + assert statusChange != null : IgniteStringFormatter.format("Missing index status change: [indexId={}, catalogStatus={}]", Review Comment: What if we run without assertions? We'll just return null and get an NPE further. How about using `Objects#requireNonNull()` instead of asserting? This doesn't seem to be a hot code. ########## modules/index/src/main/java/org/apache/ignite/internal/index/ChangeIndexStatusTask.java: ########## @@ -333,8 +335,26 @@ private <T> CompletableFuture<T> inBusyLocks(Supplier<CompletableFuture<T>> supp try { return supplier.get(); + } catch (Throwable t) { + return failedFuture(t); } finally { leaveBusy(); } } + + private MetaIndexStatusChange statusChange() { + IndexMeta indexMeta = indexMetaStorage.indexMeta(indexDescriptor.id()); + + if (indexMeta == null) { + // Index was destroyed under a low watermark, well, we need to build it. Review Comment: The comment says that the index was already destroyed, but it also says that it needs to build it. Is this a typo? Should it say that it does NOT need to build it anymore? -- 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