You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ygerzhedovich (via GitHub)" <gi...@apache.org> on 2025/01/24 15:59:24 UTC
[PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
ygerzhedovich opened a new pull request, #5121:
URL: https://github.com/apache/ignite-3/pull/5121
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933420314
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
revert the change and mute ander the https://issues.apache.org/jira/browse/IGNITE-24339
##########
.idea/codeStyles/Project.xml:
##########
Review Comment:
removed
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933420700
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
Review Comment:
thanks, done
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
Review Comment:
fixed
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933358048
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
as far as I see, this flakiness highlights the real issue, and you decided to just obscure it with this wait.
Initial version of Catalog uses `0L` as activation time, which is not valid value according to HybridTimestamp' constructor:
```
private HybridTimestamp(long time) {
this.time = time;
// Negative time breaks comparison, we don't allow overflow of the physical time.
// "0" is a reserved value for "NULL_HYBRID_TIMESTAMP".
if (time <= 0) {
throw new IllegalArgumentException("Time is out of bounds: " + time);
}
}
```
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935440293
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
+ * not been applied. Order of bits the same as commands given for execution to Catalog.
+ * @param catalogVersion Version of a catalog when the changes was visible.
+ */
+ public CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
+ assert appliedResults != null;
+
+ this.applied = appliedResults;
+ this.catalogVersion = catalogVersion;
+ this.catalogTime = catalogTime;
+ }
+
+ /** Returns catalog version since applied result is available. */
+ public int getCatalogVersion() {
+ return catalogVersion;
+ }
+
+ /** Returns catalog time since applied result is available. */
+ public long getCatalogTime() {
+
Review Comment:
thanks
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
+ * not been applied. Order of bits the same as commands given for execution to Catalog.
+ * @param catalogVersion Version of a catalog when the changes was visible.
+ */
+ public CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
+ assert appliedResults != null;
+
+ this.applied = appliedResults;
+ this.catalogVersion = catalogVersion;
+ this.catalogTime = catalogTime;
+ }
+
+ /** Returns catalog version since applied result is available. */
+ public int getCatalogVersion() {
+ return catalogVersion;
+ }
+
+ /** Returns catalog time since applied result is available. */
+ public long getCatalogTime() {
+
+ return catalogTime;
+ }
+
+ /**
+ * Returns has been applied command by given index or not.
Review Comment:
ok
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933954652
##########
modules/core/src/main/java/org/apache/ignite/internal/hlc/HybridTimestamp.java:
##########
@@ -50,7 +50,7 @@ public final class HybridTimestamp implements Comparable<HybridTimestamp>, Seria
private static final long serialVersionUID = -4285668148196042529L;
/** Time value to store for {@code null} hybrid timestamp values. */
- public static final long NULL_HYBRID_TIMESTAMP = 0L;
+ public static final long NULL_HYBRID_TIMESTAMP = 1L;
Review Comment:
reverted
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1932460706
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
assert catalog != null;
- return catalog.time();
- } else if (ignoreExpectedError) {
- Throwable err0 = err instanceof CompletionException ? err.getCause() : err;
-
- if (expErrCls.isAssignableFrom(err0.getClass())) {
- return null;
- }
+ return applyResult.isApplied(0) ? catalog.time() : null;
Review Comment:
Why we need this? :thinking:
Previously we did not return the time if the command was not applied.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933405831
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
assert catalog != null;
- return catalog.time();
- } else if (ignoreExpectedError) {
- Throwable err0 = err instanceof CompletionException ? err.getCause() : err;
-
- if (expErrCls.isAssignableFrom(err0.getClass())) {
- return null;
- }
+ return applyResult.isApplied(0) ? catalog.time() : null;
Review Comment:
It seems you are right, but it should be a separate change, because we rely on the contract in our Cursor for flag wasApplied
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935130711
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
Review Comment:
```suggestion
* @param appliedResults Set of bits representing the result of applying commands. Every {@code 1} bit indicates that the corresponding command was applied, {@code 0} indicates that the corresponding command has
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
+ * not been applied. Order of bits the same as commands given for execution to Catalog.
+ * @param catalogVersion Version of a catalog when the changes was visible.
+ */
+ public CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
+ assert appliedResults != null;
+
+ this.applied = appliedResults;
+ this.catalogVersion = catalogVersion;
+ this.catalogTime = catalogTime;
+ }
+
+ /** Returns catalog version since applied result is available. */
+ public int getCatalogVersion() {
+ return catalogVersion;
+ }
+
+ /** Returns catalog time since applied result is available. */
+ public long getCatalogTime() {
+
+ return catalogTime;
+ }
+
+ /**
+ * Returns has been applied command by given index or not.
Review Comment:
```suggestion
* Returns whether the specified command was applied or not.
```
:thinking:
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
+ * not been applied. Order of bits the same as commands given for execution to Catalog.
+ * @param catalogVersion Version of a catalog when the changes was visible.
+ */
+ public CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
Review Comment:
suggestion from IDEA :thinking:
```suggestion
CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
+ * not been applied. Order of bits the same as commands given for execution to Catalog.
+ * @param catalogVersion Version of a catalog when the changes was visible.
+ */
+ public CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
+ assert appliedResults != null;
+
+ this.applied = appliedResults;
+ this.catalogVersion = catalogVersion;
+ this.catalogTime = catalogTime;
+ }
+
+ /** Returns catalog version since applied result is available. */
+ public int getCatalogVersion() {
+ return catalogVersion;
+ }
+
+ /** Returns catalog time since applied result is available. */
+ public long getCatalogTime() {
+
Review Comment:
```suggestion
```
extra line
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
Review Comment:
```suggestion
* Creates a result with a bitset of applied results and version of a catalog when the changes was visible.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -395,17 +397,32 @@ private CompletableFuture<Integer> saveUpdate(UpdateProducer updateProducer, int
Catalog catalog = catalogByVer.lastEntry().getValue();
- List<UpdateEntry> updates;
+ BitSet applyResults = new BitSet(batchUpdateProducers.size());
+ List<UpdateEntry> bulkUpdateEntries = new ArrayList<>();
try {
- updates = updateProducer.get(new UpdateContext(catalog));
+ UpdateContext updateContext = new UpdateContext(catalog);
+ for (int i = 0; i < batchUpdateProducers.size(); i++) {
+ UpdateProducer update = batchUpdateProducers.get(i);
+ List<UpdateEntry> entries = update.get(updateContext);
+
+ for (UpdateEntry entry : entries) {
+ updateContext.updateCatalog(cat -> entry.applyUpdate(cat, INITIAL_CAUSALITY_TOKEN));
+ }
+
+ if (!entries.isEmpty()) {
+ applyResults.set(i);
+ bulkUpdateEntries.addAll(entries);
+ }
+ }
} catch (CatalogValidationException ex) {
return failedFuture(new CatalogVersionAwareValidationException(ex, catalog.version()));
} catch (Exception ex) {
return failedFuture(ex);
}
- if (updates.isEmpty()) {
- return completedFuture(catalog.version());
+ // all results are empty.
+ if (applyResults.cardinality() == 0) {
Review Comment:
```suggestion
if (bulkUpdateEntries.isEmpty()) {
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -376,14 +377,15 @@ private HybridTimestamp calcClusterWideEnsureActivationTime(Catalog catalog) {
}
/**
- * Attempts to save a versioned update using a CAS-like logic. If the attempt fails, makes more attempts
- * until the max retry count is reached.
+ * Attempts to save a versioned update using a CAS-like logic. If the attempt fails, makes more attempts until the max retry count is
+ * reached.
*
* @param updateProducer Supplies simple updates to include into a versioned update to install.
* @param attemptNo Ordinal number of an attempt.
- * @return Future that completes with the new Catalog version (if update was saved successfully) or an exception, otherwise.
+ * @return Future that completes with the result of applying updates, contains the Catalog version when the updates are visible or an
+ * exception in case of any error.
*/
- private CompletableFuture<Integer> saveUpdate(UpdateProducer updateProducer, int attemptNo) {
+ private CompletableFuture<CatalogApplyResult> saveUpdate(List<UpdateProducer> batchUpdateProducers, int attemptNo) {
Review Comment:
Javadoc parameter `updateProducer` should also be renamed.
p.s. personally I would just leave it as `updateProducers` :thinking:
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
+ * not been applied. Order of bits the same as commands given for execution to Catalog.
+ * @param catalogVersion Version of a catalog when the changes was visible.
+ */
+ public CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
+ assert appliedResults != null;
+
+ this.applied = appliedResults;
+ this.catalogVersion = catalogVersion;
+ this.catalogTime = catalogTime;
+ }
+
+ /** Returns catalog version since applied result is available. */
+ public int getCatalogVersion() {
+ return catalogVersion;
+ }
+
+ /** Returns catalog time since applied result is available. */
+ public long getCatalogTime() {
+
+ return catalogTime;
+ }
+
+ /**
+ * Returns has been applied command by given index or not.
+ *
+ * @param idx Index of command in the result.
+ * @return By given index return {@code true} if command has been successfully applied or {@code false} otherwise.
Review Comment:
```suggestion
* @return {@code True} if command has been successfully applied or {@code false} otherwise.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -395,17 +397,32 @@ private CompletableFuture<Integer> saveUpdate(UpdateProducer updateProducer, int
Catalog catalog = catalogByVer.lastEntry().getValue();
- List<UpdateEntry> updates;
+ BitSet applyResults = new BitSet(batchUpdateProducers.size());
+ List<UpdateEntry> bulkUpdateEntries = new ArrayList<>();
try {
- updates = updateProducer.get(new UpdateContext(catalog));
+ UpdateContext updateContext = new UpdateContext(catalog);
+ for (int i = 0; i < batchUpdateProducers.size(); i++) {
+ UpdateProducer update = batchUpdateProducers.get(i);
+ List<UpdateEntry> entries = update.get(updateContext);
+
+ for (UpdateEntry entry : entries) {
+ updateContext.updateCatalog(cat -> entry.applyUpdate(cat, INITIAL_CAUSALITY_TOKEN));
+ }
+
+ if (!entries.isEmpty()) {
+ applyResults.set(i);
+ bulkUpdateEntries.addAll(entries);
+ }
Review Comment:
```suggestion
if (entries.isEmpty()) {
continue;
}
for (UpdateEntry entry : entries) {
updateContext.updateCatalog(cat -> entry.applyUpdate(cat, INITIAL_CAUSALITY_TOKEN));
}
applyResults.set(i);
bulkUpdateEntries.addAll(entries);
```
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1930567998
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
we had flaky here
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933358048
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
as far as I see, this flakiness highlights the real issue, and you decided to just obscure it with this wait.
Initial version of Catalog uses `0L` as activation time, which is not valid value according to HybridTimestamp's constructor:
```
private HybridTimestamp(long time) {
this.time = time;
// Negative time breaks comparison, we don't allow overflow of the physical time.
// "0" is a reserved value for "NULL_HYBRID_TIMESTAMP".
if (time <= 0) {
throw new IllegalArgumentException("Time is out of bounds: " + time);
}
}
```
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933420314
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
revert the change and mute ander the https://issues.apache.org/jira/browse/IGNITE-24339
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
thanks, fixed
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1932460706
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
assert catalog != null;
- return catalog.time();
- } else if (ignoreExpectedError) {
- Throwable err0 = err instanceof CompletionException ? err.getCause() : err;
-
- if (expErrCls.isAssignableFrom(err0.getClass())) {
- return null;
- }
+ return applyResult.isApplied(0) ? catalog.time() : null;
Review Comment:
@korlov42, why do we need this? :thinking:
Previously we did not return the time if the command was not applied.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935142743
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
+ * not been applied. Order of bits the same as commands given for execution to Catalog.
+ * @param catalogVersion Version of a catalog when the changes was visible.
+ */
+ public CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
+ assert appliedResults != null;
+
+ this.applied = appliedResults;
+ this.catalogVersion = catalogVersion;
+ this.catalogTime = catalogTime;
+ }
+
+ /** Returns catalog version since applied result is available. */
+ public int getCatalogVersion() {
+ return catalogVersion;
+ }
+
+ /** Returns catalog time since applied result is available. */
+ public long getCatalogTime() {
+
+ return catalogTime;
+ }
+
+ /**
+ * Returns has been applied command by given index or not.
+ *
+ * @param idx Index of command in the result.
+ * @return By given index return {@code true} if command has been successfully applied or {@code false} otherwise.
Review Comment:
```suggestion
* @return {@code True} if command has been successfully applied or {@code false} otherwise.
```
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935440688
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
+ * not been applied. Order of bits the same as commands given for execution to Catalog.
+ * @param catalogVersion Version of a catalog when the changes was visible.
+ */
+ public CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
+ assert appliedResults != null;
+
+ this.applied = appliedResults;
+ this.catalogVersion = catalogVersion;
+ this.catalogTime = catalogTime;
+ }
+
+ /** Returns catalog version since applied result is available. */
+ public int getCatalogVersion() {
+ return catalogVersion;
+ }
+
+ /** Returns catalog time since applied result is available. */
+ public long getCatalogTime() {
+
+ return catalogTime;
+ }
+
+ /**
+ * Returns has been applied command by given index or not.
+ *
+ * @param idx Index of command in the result.
+ * @return By given index return {@code true} if command has been successfully applied or {@code false} otherwise.
Review Comment:
ok
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
Review Comment:
thanks
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935476912
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -97,152 +73,34 @@ public DdlCommandHandler(
* catalog, the result will be {@code null}.
*/
public CompletableFuture<@Nullable Long> handle(CatalogCommand cmd) {
- if (cmd instanceof CreateSchemaCommand) {
- return handleCreateSchema((CreateSchemaCommand) cmd);
- } else if (cmd instanceof DropSchemaCommand) {
- return handleDropSchema((DropSchemaCommand) cmd);
- } else if (cmd instanceof CreateTableCommand) {
- return handleCreateTable((CreateTableCommand) cmd);
- } else if (cmd instanceof DropTableCommand) {
- return handleDropTable((DropTableCommand) cmd);
- } else if (cmd instanceof AlterTableAddColumnCommand) {
- return handleAlterAddColumn((AlterTableAddColumnCommand) cmd);
- } else if (cmd instanceof AlterTableDropColumnCommand) {
- return handleAlterDropColumn((AlterTableDropColumnCommand) cmd);
- } else if (cmd instanceof AlterTableAlterColumnCommand) {
- return handleAlterColumn((AlterTableAlterColumnCommand) cmd);
- } else if (cmd instanceof AbstractCreateIndexCommand) {
- return handleCreateIndex((AbstractCreateIndexCommand) cmd);
- } else if (cmd instanceof DropIndexCommand) {
- return handleDropIndex((DropIndexCommand) cmd);
- } else if (cmd instanceof CreateZoneCommand) {
- return handleCreateZone((CreateZoneCommand) cmd);
- } else if (cmd instanceof RenameZoneCommand) {
- return handleRenameZone((RenameZoneCommand) cmd);
- } else if (cmd instanceof AlterZoneCommand) {
- return handleAlterZone((AlterZoneCommand) cmd);
- } else if (cmd instanceof AlterZoneSetDefaultCommand) {
- return handleAlterZoneSetDefault((AlterZoneSetDefaultCommand) cmd);
- } else if (cmd instanceof DropZoneCommand) {
- return handleDropZone((DropZoneCommand) cmd);
- } else {
- return failedFuture(new SqlException(STMT_VALIDATION_ERR, "Unsupported DDL operation ["
- + "cmdName=" + (cmd == null ? null : cmd.getClass().getSimpleName()) + "; "
- + "cmd=\"" + cmd + "\"]"));
- }
- }
-
- /** Handles create distribution zone command. */
- private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
- }
-
- /** Handles rename zone command. */
- private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles alter zone command. */
- private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles alter zone set default command. */
- private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
+ CompletableFuture<CatalogApplyResult> fut = catalogManager.execute(cmd);
- /** Handles drop distribution zone command. */
- private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles create table command. */
- private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
- }
-
- /** Handles drop table command. */
- private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles add column command. */
- private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles drop column command. */
- private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles drop column command. */
- private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles create index command. */
- private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
- }
-
- /** Handles drop index command. */
- private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
- }
-
- /** Handles create schema command. */
- private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
- }
-
- /** Handles drop schema command. */
- private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
- }
+ if (cmd instanceof AbstractCreateIndexCommand) {
+ fut = fut.thenCompose(applyResult ->
+ inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved((AbstractCreateIndexCommand) cmd, applyResult)));
+ }
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ return fut.handle((applyResult, err) -> {
Review Comment:
done
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich merged PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935441747
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -395,17 +397,32 @@ private CompletableFuture<Integer> saveUpdate(UpdateProducer updateProducer, int
Catalog catalog = catalogByVer.lastEntry().getValue();
- List<UpdateEntry> updates;
+ BitSet applyResults = new BitSet(batchUpdateProducers.size());
+ List<UpdateEntry> bulkUpdateEntries = new ArrayList<>();
try {
- updates = updateProducer.get(new UpdateContext(catalog));
+ UpdateContext updateContext = new UpdateContext(catalog);
+ for (int i = 0; i < batchUpdateProducers.size(); i++) {
+ UpdateProducer update = batchUpdateProducers.get(i);
+ List<UpdateEntry> entries = update.get(updateContext);
+
+ for (UpdateEntry entry : entries) {
+ updateContext.updateCatalog(cat -> entry.applyUpdate(cat, INITIAL_CAUSALITY_TOKEN));
+ }
+
+ if (!entries.isEmpty()) {
+ applyResults.set(i);
+ bulkUpdateEntries.addAll(entries);
+ }
+ }
} catch (CatalogValidationException ex) {
return failedFuture(new CatalogVersionAwareValidationException(ex, catalog.version()));
} catch (Exception ex) {
return failedFuture(ex);
}
- if (updates.isEmpty()) {
- return completedFuture(catalog.version());
+ // all results are empty.
+ if (applyResults.cardinality() == 0) {
Review Comment:
it has different semantics
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935230947
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -423,6 +424,21 @@ void bulkUpdateIncrementsVersionByOne() {
@Test
void bulkUpdateDoesntIncrementVersionInCaseOfError() {
int versionBefore = manager.latestCatalogVersion();
+ tryApplyAndCheckExpect(
+ List.of(TestCommand.empty(), TestCommand.ok(), TestCommand.empty(), TestCommand.ok(), TestCommand.empty()),
+ false, true, false, true, false
+ );
+
+ Catalog updatedCatalog = manager.catalog(manager.latestCatalogVersion());
+
+ assertNotNull(updatedCatalog);
+
+ assertEquals(4 + versionBefore, updatedCatalog.objectIdGenState());
Review Comment:
How does `objectIdGenState` relate to `versionBefore`?
I think this test should be reworked
1. save `catalog before`
2. apply updates
3. check that new `catalog version` is increased by `1`
4. check that `objectIdGenState` is increased by `2`
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933952934
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
fix reverted back and the test muted under ticket.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933769840
##########
modules/core/src/main/java/org/apache/ignite/internal/hlc/HybridTimestamp.java:
##########
@@ -50,7 +50,7 @@ public final class HybridTimestamp implements Comparable<HybridTimestamp>, Seria
private static final long serialVersionUID = -4285668148196042529L;
/** Time value to store for {@code null} hybrid timestamp values. */
- public static final long NULL_HYBRID_TIMESTAMP = 0L;
+ public static final long NULL_HYBRID_TIMESTAMP = 1L;
Review Comment:
this change doesn't seem correct
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933767918
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AbstractCreateIndexCommand.java:
##########
@@ -83,9 +83,13 @@ public List<UpdateEntry> get(UpdateContext context) {
Catalog catalog = context.catalog();
CatalogSchemaDescriptor schema = schemaOrThrow(catalog, schemaName);
+ if (ifNotExists && schema.aliveIndex(indexName) != null) {
+ return List.of();
+ }
+
ensureNoTableIndexOrSysViewExistsWithGivenName(schema, indexName);
- CatalogTableDescriptor table = tableOrThrow(schema, tableName);
+ CatalogTableDescriptor table = tableOrThrow(schema, tableName, false).orElseThrow();
Review Comment:
tbh, this line looks ridiculous now: give me table, or throw exception if table doesn't exists (this semantic derived from method's name), but don't throw an exception if table doesn't exists (this one derived from `shouldThrowIfNotExists=false` parameter), and throw if table doesn't exists (call to Optional's `orElseThrow()`).
We have to stick with only one approach:
Either add new method `@Nullable CatalogTableDescriptor tableOpt(CatalogSchemaDescriptor schema, String name)` and use it wherever appropriate
or
change method to `@Nullable CatalogTableDescriptor table(CatalogSchemaDescriptor schema, String name, boolean shouldThrowIfNotExists)`
or
change method to `Optional<CatalogTableDescriptor> table(CatalogSchemaDescriptor schema, String name)`
I don't think any mixture of these may provide decent result
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935441290
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -395,17 +397,32 @@ private CompletableFuture<Integer> saveUpdate(UpdateProducer updateProducer, int
Catalog catalog = catalogByVer.lastEntry().getValue();
- List<UpdateEntry> updates;
+ BitSet applyResults = new BitSet(batchUpdateProducers.size());
+ List<UpdateEntry> bulkUpdateEntries = new ArrayList<>();
try {
- updates = updateProducer.get(new UpdateContext(catalog));
+ UpdateContext updateContext = new UpdateContext(catalog);
+ for (int i = 0; i < batchUpdateProducers.size(); i++) {
+ UpdateProducer update = batchUpdateProducers.get(i);
+ List<UpdateEntry> entries = update.get(updateContext);
+
+ for (UpdateEntry entry : entries) {
+ updateContext.updateCatalog(cat -> entry.applyUpdate(cat, INITIAL_CAUSALITY_TOKEN));
+ }
+
+ if (!entries.isEmpty()) {
+ applyResults.set(i);
+ bulkUpdateEntries.addAll(entries);
+ }
Review Comment:
agree
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935230947
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -423,6 +424,21 @@ void bulkUpdateIncrementsVersionByOne() {
@Test
void bulkUpdateDoesntIncrementVersionInCaseOfError() {
int versionBefore = manager.latestCatalogVersion();
+ tryApplyAndCheckExpect(
+ List.of(TestCommand.empty(), TestCommand.ok(), TestCommand.empty(), TestCommand.ok(), TestCommand.empty()),
+ false, true, false, true, false
+ );
+
+ Catalog updatedCatalog = manager.catalog(manager.latestCatalogVersion());
+
+ assertNotNull(updatedCatalog);
+
+ assertEquals(4 + versionBefore, updatedCatalog.objectIdGenState());
Review Comment:
How does `objectIdGenState` relates to `versionBefore`?
I think this test should be reworked
1. save `catalog before`
2. apply update
3. check catalogVersionBefore incremented by `1`
4. check objectIdGenState incremented by `2`
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -423,6 +424,21 @@ void bulkUpdateIncrementsVersionByOne() {
@Test
void bulkUpdateDoesntIncrementVersionInCaseOfError() {
int versionBefore = manager.latestCatalogVersion();
+ tryApplyAndCheckExpect(
+ List.of(TestCommand.empty(), TestCommand.ok(), TestCommand.empty(), TestCommand.ok(), TestCommand.empty()),
+ false, true, false, true, false
+ );
+
+ Catalog updatedCatalog = manager.catalog(manager.latestCatalogVersion());
+
+ assertNotNull(updatedCatalog);
+
+ assertEquals(4 + versionBefore, updatedCatalog.objectIdGenState());
Review Comment:
How does `objectIdGenState` relates to `versionBefore`?
I think this test should be reworked
1. save `catalog before`
2. apply updates
3. check catalogVersionBefore incremented by `1`
4. check objectIdGenState incremented by `2`
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935230947
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -423,6 +424,21 @@ void bulkUpdateIncrementsVersionByOne() {
@Test
void bulkUpdateDoesntIncrementVersionInCaseOfError() {
int versionBefore = manager.latestCatalogVersion();
+ tryApplyAndCheckExpect(
+ List.of(TestCommand.empty(), TestCommand.ok(), TestCommand.empty(), TestCommand.ok(), TestCommand.empty()),
+ false, true, false, true, false
+ );
+
+ Catalog updatedCatalog = manager.catalog(manager.latestCatalogVersion());
+
+ assertNotNull(updatedCatalog);
+
+ assertEquals(4 + versionBefore, updatedCatalog.objectIdGenState());
Review Comment:
How does `objectIdGenState` relates to `versionBefore`?
I think this test should be reworked
1. save `catalog before`
2. apply updates
3. check that `catalog version` is incremented by `1`
4. check that `objectIdGenState` is incremented by `2`
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935230947
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -423,6 +424,21 @@ void bulkUpdateIncrementsVersionByOne() {
@Test
void bulkUpdateDoesntIncrementVersionInCaseOfError() {
int versionBefore = manager.latestCatalogVersion();
+ tryApplyAndCheckExpect(
+ List.of(TestCommand.empty(), TestCommand.ok(), TestCommand.empty(), TestCommand.ok(), TestCommand.empty()),
+ false, true, false, true, false
+ );
+
+ Catalog updatedCatalog = manager.catalog(manager.latestCatalogVersion());
+
+ assertNotNull(updatedCatalog);
+
+ assertEquals(4 + versionBefore, updatedCatalog.objectIdGenState());
Review Comment:
How does `objectIdGenState` relate to `versionBefore`?
I think this test should be reworked
1. save `catalog before`
2. apply updates
3. check that `catalog version` is increased by `1`
4. check that `objectIdGenState` is increased by `2`
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935440027
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
Review Comment:
done
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogApplyResult.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog;
+
+
+import java.util.BitSet;
+
+/** Represent result of applying Catalog command. */
+public class CatalogApplyResult {
+ private final int catalogVersion;
+ private final long catalogTime;
+ private final BitSet applied;
+
+ /**
+ * Creates an result with a bitset of applied results and version of a catalog when the changes was visible.
+ *
+ * @param appliedResults Result of applying of commands. Every 1 bit say about applied result, otherwise 0 says that command has
+ * not been applied. Order of bits the same as commands given for execution to Catalog.
+ * @param catalogVersion Version of a catalog when the changes was visible.
+ */
+ public CatalogApplyResult(BitSet appliedResults, int catalogVersion, long catalogTime) {
Review Comment:
done
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933367687
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
assert catalog != null;
- return catalog.time();
- } else if (ignoreExpectedError) {
- Throwable err0 = err instanceof CompletionException ? err.getCause() : err;
-
- if (expErrCls.isAssignableFrom(err0.getClass())) {
- return null;
- }
+ return applyResult.isApplied(0) ? catalog.time() : null;
Review Comment:
assume the same client executes the sequence of commands:
```
CREATE TABLE IF NOT EXISTS my_table ...;
SELECT COUNT(*) FROM my_table;
```
Current time is `Tnow`, current observed time `Tobs`, and `dT` is the maximum allowed offset for read-in-the-past transaction.
If DDL statements results in no-op, and activation time of the version containing the table is after `max(Tobs, Tnow - dT)`, then second statement will fail with `TABLE NOT FOUND`, which must never happen in the absence of concurrent `DROP`'s. To address the issue, we need to always update `Tobs`.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933421102
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
Review Comment:
ok, done
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1932288663
##########
.idea/codeStyles/Project.xml:
##########
Review Comment:
unrelated changes
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
Review Comment:
no need to return BiFuction anymore, the whole method may become BiFunction
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
Review Comment:
given that catalogManager now returns an object, let's just put activation time there rather that resolve it afterward
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
Review Comment:
why don't you remove all this `handleSomthing` methods? Now they are literally the same (except for `handleCreateIndex`)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
are you talking about `testZoneNotFoundOnDrop2`?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
assert catalog != null;
- return catalog.time();
- } else if (ignoreExpectedError) {
- Throwable err0 = err instanceof CompletionException ? err.getCause() : err;
-
- if (expErrCls.isAssignableFrom(err0.getClass())) {
- return null;
- }
+ return applyResult.isApplied(0) ? catalog.time() : null;
Review Comment:
I thin, we need to return activation time every time command completes successfully (even if command itself resulted in no-op)
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1932413903
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
yes
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1932460706
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
assert catalog != null;
- return catalog.time();
- } else if (ignoreExpectedError) {
- Throwable err0 = err instanceof CompletionException ? err.getCause() : err;
-
- if (expErrCls.isAssignableFrom(err0.getClass())) {
- return null;
- }
+ return applyResult.isApplied(0) ? catalog.time() : null;
Review Comment:
@korlov42 Why do we need this? :thinking:
Previously we did not return the time if the command was not applied.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1933937426
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AbstractCreateIndexCommand.java:
##########
@@ -83,9 +83,13 @@ public List<UpdateEntry> get(UpdateContext context) {
Catalog catalog = context.catalog();
CatalogSchemaDescriptor schema = schemaOrThrow(catalog, schemaName);
+ if (ifNotExists && schema.aliveIndex(indexName) != null) {
+ return List.of();
+ }
+
ensureNoTableIndexOrSysViewExistsWithGivenName(schema, indexName);
- CatalogTableDescriptor table = tableOrThrow(schema, tableName);
+ CatalogTableDescriptor table = tableOrThrow(schema, tableName, false).orElseThrow();
Review Comment:
agree. amended to the second suggested way
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935441081
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -376,14 +377,15 @@ private HybridTimestamp calcClusterWideEnsureActivationTime(Catalog catalog) {
}
/**
- * Attempts to save a versioned update using a CAS-like logic. If the attempt fails, makes more attempts
- * until the max retry count is reached.
+ * Attempts to save a versioned update using a CAS-like logic. If the attempt fails, makes more attempts until the max retry count is
+ * reached.
*
* @param updateProducer Supplies simple updates to include into a versioned update to install.
* @param attemptNo Ordinal number of an attempt.
- * @return Future that completes with the new Catalog version (if update was saved successfully) or an exception, otherwise.
+ * @return Future that completes with the result of applying updates, contains the Catalog version when the updates are visible or an
+ * exception in case of any error.
*/
- private CompletableFuture<Integer> saveUpdate(UpdateProducer updateProducer, int attemptNo) {
+ private CompletableFuture<CatalogApplyResult> saveUpdate(List<UpdateProducer> batchUpdateProducers, int attemptNo) {
Review Comment:
ok
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935442120
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -423,6 +424,21 @@ void bulkUpdateIncrementsVersionByOne() {
@Test
void bulkUpdateDoesntIncrementVersionInCaseOfError() {
int versionBefore = manager.latestCatalogVersion();
+ tryApplyAndCheckExpect(
+ List.of(TestCommand.empty(), TestCommand.ok(), TestCommand.empty(), TestCommand.ok(), TestCommand.empty()),
+ false, true, false, true, false
+ );
+
+ Catalog updatedCatalog = manager.catalog(manager.latestCatalogVersion());
+
+ assertNotNull(updatedCatalog);
+
+ assertEquals(4 + versionBefore, updatedCatalog.objectIdGenState());
Review Comment:
reworked these tests
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935313319
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -97,152 +73,34 @@ public DdlCommandHandler(
* catalog, the result will be {@code null}.
*/
public CompletableFuture<@Nullable Long> handle(CatalogCommand cmd) {
- if (cmd instanceof CreateSchemaCommand) {
- return handleCreateSchema((CreateSchemaCommand) cmd);
- } else if (cmd instanceof DropSchemaCommand) {
- return handleDropSchema((DropSchemaCommand) cmd);
- } else if (cmd instanceof CreateTableCommand) {
- return handleCreateTable((CreateTableCommand) cmd);
- } else if (cmd instanceof DropTableCommand) {
- return handleDropTable((DropTableCommand) cmd);
- } else if (cmd instanceof AlterTableAddColumnCommand) {
- return handleAlterAddColumn((AlterTableAddColumnCommand) cmd);
- } else if (cmd instanceof AlterTableDropColumnCommand) {
- return handleAlterDropColumn((AlterTableDropColumnCommand) cmd);
- } else if (cmd instanceof AlterTableAlterColumnCommand) {
- return handleAlterColumn((AlterTableAlterColumnCommand) cmd);
- } else if (cmd instanceof AbstractCreateIndexCommand) {
- return handleCreateIndex((AbstractCreateIndexCommand) cmd);
- } else if (cmd instanceof DropIndexCommand) {
- return handleDropIndex((DropIndexCommand) cmd);
- } else if (cmd instanceof CreateZoneCommand) {
- return handleCreateZone((CreateZoneCommand) cmd);
- } else if (cmd instanceof RenameZoneCommand) {
- return handleRenameZone((RenameZoneCommand) cmd);
- } else if (cmd instanceof AlterZoneCommand) {
- return handleAlterZone((AlterZoneCommand) cmd);
- } else if (cmd instanceof AlterZoneSetDefaultCommand) {
- return handleAlterZoneSetDefault((AlterZoneSetDefaultCommand) cmd);
- } else if (cmd instanceof DropZoneCommand) {
- return handleDropZone((DropZoneCommand) cmd);
- } else {
- return failedFuture(new SqlException(STMT_VALIDATION_ERR, "Unsupported DDL operation ["
- + "cmdName=" + (cmd == null ? null : cmd.getClass().getSimpleName()) + "; "
- + "cmd=\"" + cmd + "\"]"));
- }
- }
-
- /** Handles create distribution zone command. */
- private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
- }
-
- /** Handles rename zone command. */
- private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles alter zone command. */
- private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles alter zone set default command. */
- private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
+ CompletableFuture<CatalogApplyResult> fut = catalogManager.execute(cmd);
- /** Handles drop distribution zone command. */
- private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles create table command. */
- private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
- }
-
- /** Handles drop table command. */
- private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles add column command. */
- private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles drop column command. */
- private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles drop column command. */
- private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles create index command. */
- private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
- }
-
- /** Handles drop index command. */
- private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
- }
-
- /** Handles create schema command. */
- private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
- }
-
- /** Handles drop schema command. */
- private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
- }
+ if (cmd instanceof AbstractCreateIndexCommand) {
+ fut = fut.thenCompose(applyResult ->
+ inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved((AbstractCreateIndexCommand) cmd, applyResult)));
+ }
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ return fut.handle((applyResult, err) -> {
Review Comment:
return fut.thenApply(applyResult -> applyResult.isApplied(0) ? applyResult.getCatalogTime() : null);
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -97,152 +73,34 @@ public DdlCommandHandler(
* catalog, the result will be {@code null}.
*/
public CompletableFuture<@Nullable Long> handle(CatalogCommand cmd) {
- if (cmd instanceof CreateSchemaCommand) {
- return handleCreateSchema((CreateSchemaCommand) cmd);
- } else if (cmd instanceof DropSchemaCommand) {
- return handleDropSchema((DropSchemaCommand) cmd);
- } else if (cmd instanceof CreateTableCommand) {
- return handleCreateTable((CreateTableCommand) cmd);
- } else if (cmd instanceof DropTableCommand) {
- return handleDropTable((DropTableCommand) cmd);
- } else if (cmd instanceof AlterTableAddColumnCommand) {
- return handleAlterAddColumn((AlterTableAddColumnCommand) cmd);
- } else if (cmd instanceof AlterTableDropColumnCommand) {
- return handleAlterDropColumn((AlterTableDropColumnCommand) cmd);
- } else if (cmd instanceof AlterTableAlterColumnCommand) {
- return handleAlterColumn((AlterTableAlterColumnCommand) cmd);
- } else if (cmd instanceof AbstractCreateIndexCommand) {
- return handleCreateIndex((AbstractCreateIndexCommand) cmd);
- } else if (cmd instanceof DropIndexCommand) {
- return handleDropIndex((DropIndexCommand) cmd);
- } else if (cmd instanceof CreateZoneCommand) {
- return handleCreateZone((CreateZoneCommand) cmd);
- } else if (cmd instanceof RenameZoneCommand) {
- return handleRenameZone((RenameZoneCommand) cmd);
- } else if (cmd instanceof AlterZoneCommand) {
- return handleAlterZone((AlterZoneCommand) cmd);
- } else if (cmd instanceof AlterZoneSetDefaultCommand) {
- return handleAlterZoneSetDefault((AlterZoneSetDefaultCommand) cmd);
- } else if (cmd instanceof DropZoneCommand) {
- return handleDropZone((DropZoneCommand) cmd);
- } else {
- return failedFuture(new SqlException(STMT_VALIDATION_ERR, "Unsupported DDL operation ["
- + "cmdName=" + (cmd == null ? null : cmd.getClass().getSimpleName()) + "; "
- + "cmd=\"" + cmd + "\"]"));
- }
- }
-
- /** Handles create distribution zone command. */
- private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
- }
-
- /** Handles rename zone command. */
- private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles alter zone command. */
- private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles alter zone set default command. */
- private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
+ CompletableFuture<CatalogApplyResult> fut = catalogManager.execute(cmd);
- /** Handles drop distribution zone command. */
- private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles create table command. */
- private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
- }
-
- /** Handles drop table command. */
- private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles add column command. */
- private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles drop column command. */
- private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles drop column command. */
- private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles create index command. */
- private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
- }
-
- /** Handles drop index command. */
- private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
- }
-
- /** Handles create schema command. */
- private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
- }
-
- /** Handles drop schema command. */
- private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
- }
+ if (cmd instanceof AbstractCreateIndexCommand) {
+ fut = fut.thenCompose(applyResult ->
+ inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved((AbstractCreateIndexCommand) cmd, applyResult)));
+ }
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ return fut.handle((applyResult, err) -> {
Review Comment:
```suggestion
return fut.thenApply(applyResult -> applyResult.isApplied(0) ? applyResult.getCatalogTime() : null);
```
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935313319
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -97,152 +73,34 @@ public DdlCommandHandler(
* catalog, the result will be {@code null}.
*/
public CompletableFuture<@Nullable Long> handle(CatalogCommand cmd) {
- if (cmd instanceof CreateSchemaCommand) {
- return handleCreateSchema((CreateSchemaCommand) cmd);
- } else if (cmd instanceof DropSchemaCommand) {
- return handleDropSchema((DropSchemaCommand) cmd);
- } else if (cmd instanceof CreateTableCommand) {
- return handleCreateTable((CreateTableCommand) cmd);
- } else if (cmd instanceof DropTableCommand) {
- return handleDropTable((DropTableCommand) cmd);
- } else if (cmd instanceof AlterTableAddColumnCommand) {
- return handleAlterAddColumn((AlterTableAddColumnCommand) cmd);
- } else if (cmd instanceof AlterTableDropColumnCommand) {
- return handleAlterDropColumn((AlterTableDropColumnCommand) cmd);
- } else if (cmd instanceof AlterTableAlterColumnCommand) {
- return handleAlterColumn((AlterTableAlterColumnCommand) cmd);
- } else if (cmd instanceof AbstractCreateIndexCommand) {
- return handleCreateIndex((AbstractCreateIndexCommand) cmd);
- } else if (cmd instanceof DropIndexCommand) {
- return handleDropIndex((DropIndexCommand) cmd);
- } else if (cmd instanceof CreateZoneCommand) {
- return handleCreateZone((CreateZoneCommand) cmd);
- } else if (cmd instanceof RenameZoneCommand) {
- return handleRenameZone((RenameZoneCommand) cmd);
- } else if (cmd instanceof AlterZoneCommand) {
- return handleAlterZone((AlterZoneCommand) cmd);
- } else if (cmd instanceof AlterZoneSetDefaultCommand) {
- return handleAlterZoneSetDefault((AlterZoneSetDefaultCommand) cmd);
- } else if (cmd instanceof DropZoneCommand) {
- return handleDropZone((DropZoneCommand) cmd);
- } else {
- return failedFuture(new SqlException(STMT_VALIDATION_ERR, "Unsupported DDL operation ["
- + "cmdName=" + (cmd == null ? null : cmd.getClass().getSimpleName()) + "; "
- + "cmd=\"" + cmd + "\"]"));
- }
- }
-
- /** Handles create distribution zone command. */
- private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
- }
-
- /** Handles rename zone command. */
- private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles alter zone command. */
- private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles alter zone set default command. */
- private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
+ CompletableFuture<CatalogApplyResult> fut = catalogManager.execute(cmd);
- /** Handles drop distribution zone command. */
- private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
- }
-
- /** Handles create table command. */
- private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
- }
-
- /** Handles drop table command. */
- private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles add column command. */
- private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles drop column command. */
- private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles drop column command. */
- private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
- }
-
- /** Handles create index command. */
- private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
- }
-
- /** Handles drop index command. */
- private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
- }
-
- /** Handles create schema command. */
- private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
- }
-
- /** Handles drop schema command. */
- private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
- }
+ if (cmd instanceof AbstractCreateIndexCommand) {
+ fut = fut.thenCompose(applyResult ->
+ inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved((AbstractCreateIndexCommand) cmd, applyResult)));
+ }
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ return fut.handle((applyResult, err) -> {
Review Comment:
return fut.thenApply(applyResult -> applyResult.isApplied(0) ? applyResult.getCatalogTime() : null);
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1935230947
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -423,6 +424,21 @@ void bulkUpdateIncrementsVersionByOne() {
@Test
void bulkUpdateDoesntIncrementVersionInCaseOfError() {
int versionBefore = manager.latestCatalogVersion();
+ tryApplyAndCheckExpect(
+ List.of(TestCommand.empty(), TestCommand.ok(), TestCommand.empty(), TestCommand.ok(), TestCommand.empty()),
+ false, true, false, true, false
+ );
+
+ Catalog updatedCatalog = manager.catalog(manager.latestCatalogVersion());
+
+ assertNotNull(updatedCatalog);
+
+ assertEquals(4 + versionBefore, updatedCatalog.objectIdGenState());
Review Comment:
How does `objectIdGenState` relates to `versionBefore`?
I think this test should be reworked
1. save `catalog before`
2. apply updates
3. check that `catalog version` is increased by `1`
4. check that `objectIdGenState` is increased by `2`
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1930567998
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -67,6 +71,7 @@ void before() {
HybridClock clock = new HybridClockImpl();
catalogManager = createTestCatalogManager("test", clock);
assertThat(catalogManager.startAsync(new ComponentContext()), willCompleteSuccessfully());
+ awaitDefaultZoneCreation(catalogManager);
Review Comment:
we had flaky due to the absent of the await
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1932460706
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
assert catalog != null;
- return catalog.time();
- } else if (ignoreExpectedError) {
- Throwable err0 = err instanceof CompletionException ? err.getCause() : err;
-
- if (expErrCls.isAssignableFrom(err0.getClass())) {
- return null;
- }
+ return applyResult.isApplied(0) ? catalog.time() : null;
Review Comment:
Why do we need this? :thinking:
Previously we did not return the time if the command was not applied.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ignite-22197 Move ifExists flag handling inside a Catalog [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #5121:
URL: https://github.com/apache/ignite-3/pull/5121#discussion_r1932460706
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -134,115 +127,102 @@ public DdlCommandHandler(
/** Handles create distribution zone command. */
private CompletableFuture<@Nullable Long> handleCreateZone(CreateZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), DistributionZoneExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles rename zone command. */
private CompletableFuture<@Nullable Long> handleRenameZone(RenameZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone command. */
private CompletableFuture<@Nullable Long> handleAlterZone(AlterZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles alter zone set default command. */
private CompletableFuture<@Nullable Long> handleAlterZoneSetDefault(AlterZoneSetDefaultCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop distribution zone command. */
private CompletableFuture<@Nullable Long> handleDropZone(DropZoneCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), DistributionZoneNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create table command. */
private CompletableFuture<@Nullable Long> handleCreateTable(CreateTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableExistsValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop table command. */
private CompletableFuture<@Nullable Long> handleDropTable(DropTableCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles add column command. */
private CompletableFuture<@Nullable Long> handleAlterAddColumn(AlterTableAddColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterDropColumn(AlterTableDropColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop column command. */
private CompletableFuture<@Nullable Long> handleAlterColumn(AlterTableAlterColumnCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifTableExists(), TableNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create index command. */
private CompletableFuture<@Nullable Long> handleCreateIndex(AbstractCreateIndexCommand cmd) {
return catalogManager.execute(cmd)
- .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
- .handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
+ .thenCompose(applyResult -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, applyResult)))
+ .handle(handleModificationResult());
}
/** Handles drop index command. */
private CompletableFuture<@Nullable Long> handleDropIndex(DropIndexCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), IndexNotFoundValidationException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles create schema command. */
private CompletableFuture<Long> handleCreateSchema(CreateSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifNotExists(), SchemaExistsException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
/** Handles drop schema command. */
private CompletableFuture<Long> handleDropSchema(DropSchemaCommand cmd) {
- return catalogManager.execute(cmd)
- .handle(handleModificationResult(cmd.ifExists(), SchemaNotFoundException.class));
+ return catalogManager.execute(cmd).handle(handleModificationResult());
}
- private BiFunction<Integer, Throwable, @Nullable Long> handleModificationResult(boolean ignoreExpectedError, Class<?> expErrCls) {
- return (ver, err) -> {
+ private BiFunction<CatalogApplyResult, Throwable, @Nullable Long> handleModificationResult() {
+ return (applyResult, err) -> {
if (err == null) {
- Catalog catalog = catalogManager.catalog(ver);
+ Catalog catalog = catalogManager.catalog(applyResult.getCatalogVersion());
assert catalog != null;
- return catalog.time();
- } else if (ignoreExpectedError) {
- Throwable err0 = err instanceof CompletionException ? err.getCause() : err;
-
- if (expErrCls.isAssignableFrom(err0.getClass())) {
- return null;
- }
+ return applyResult.isApplied(0) ? catalog.time() : null;
Review Comment:
Why we need this?
Previously we did not return the time if the command was not applied. :thinking:
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org