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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
