sashapolo commented on code in PR #2500:
URL: https://github.com/apache/ignite-3/pull/2500#discussion_r1313029415
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/SchemaDistributedConfigurationModuleTest.java:
##########
@@ -45,24 +41,22 @@ void typeIsDistributed() {
@Test
void providesTableValidator() {
- assertThat(module.validators(),
hasItem(instanceOf(TableValidatorImpl.class)));
+ assertThat(module.validators(), empty());
}
@Test
void providesColumnTypeValidator() {
- assertThat(module.validators(),
hasItem(instanceOf(ColumnTypeValidatorImpl.class)));
+ assertThat(module.validators(), empty());
}
@Test
void providesInternalSchemaExtensions() {
- assertThat(module.schemaExtensions(),
hasItem(ExtendedTableConfigurationSchema.class));
+ assertThat(module.schemaExtensions(), empty());
}
@Test
void providesNoPolymorphicSchemaExtensions() {
- assertThat(module.polymorphicSchemaExtensions(),
hasItem(ConstantValueDefaultConfigurationSchema.class));
- assertThat(module.polymorphicSchemaExtensions(),
hasItem(FunctionCallDefaultConfigurationSchema.class));
- assertThat(module.polymorphicSchemaExtensions(),
hasItem(NullValueDefaultConfigurationSchema.class));
+ assertThat(module.polymorphicSchemaExtensions(), empty());
Review Comment:
What's the point of these tests now?
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TableTestUtils.java:
##########
@@ -61,6 +62,46 @@ public static void createTable(
assertThat(catalogManager.createTable(builder.build()),
willCompleteSuccessfully());
}
+ /**
+ * Drops table in the catalog.
+ *
+ * @param catalogManager Catalog manager.
+ * @param schemaName Schema name.
+ * @param tableName Table name.
+ */
+ public static void dropTable(CatalogManager catalogManager, String
schemaName, String tableName) {
+ assertThat(
+
catalogManager.dropTable(DropTableParams.builder().schemaName(schemaName).tableName(tableName).build()),
+ willCompleteSuccessfully()
+ );
+ }
+
+ /**
+ * Returns table descriptor form catalog, {@code null} if table is absent.
+ *
+ * @param catalogService Catalog service.
+ * @param tableName Table name.
+ * @param timestamp Timestamp.
+ */
+ public static @Nullable CatalogTableDescriptor getTable(CatalogService
catalogService, String tableName, long timestamp) {
+ return catalogService.table(tableName, timestamp);
+ }
+
+ /**
+ * Returns table descriptor form catalog, {@code null} if table is absent.
Review Comment:
This is not true
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TableTestUtils.java:
##########
@@ -61,6 +62,46 @@ public static void createTable(
assertThat(catalogManager.createTable(builder.build()),
willCompleteSuccessfully());
}
+ /**
+ * Drops table in the catalog.
+ *
+ * @param catalogManager Catalog manager.
+ * @param schemaName Schema name.
+ * @param tableName Table name.
+ */
+ public static void dropTable(CatalogManager catalogManager, String
schemaName, String tableName) {
+ assertThat(
+
catalogManager.dropTable(DropTableParams.builder().schemaName(schemaName).tableName(tableName).build()),
+ willCompleteSuccessfully()
+ );
+ }
+
+ /**
+ * Returns table descriptor form catalog, {@code null} if table is absent.
+ *
+ * @param catalogService Catalog service.
+ * @param tableName Table name.
+ * @param timestamp Timestamp.
+ */
+ public static @Nullable CatalogTableDescriptor getTable(CatalogService
catalogService, String tableName, long timestamp) {
+ return catalogService.table(tableName, timestamp);
+ }
+
+ /**
+ * Returns table descriptor form catalog, {@code null} if table is absent.
+ *
+ * @param catalogService Catalog service.
+ * @param tableName Table name.
+ * @param timestamp Timestamp.
+ */
+ public static CatalogTableDescriptor getTableStrict(CatalogService
catalogService, String tableName, long timestamp) {
Review Comment:
Usually such methods are called something like `getRequiredTable`
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2013,94 +1589,67 @@ public CompletableFuture<TableImpl>
tableImplAsync(String name) {
* @return Future representing pending completion of the {@code
TableManager#tableAsyncInternal} operation.
*/
public CompletableFuture<TableImpl> tableAsyncInternal(String name) {
- if (!busyLock.enterBusy()) {
- throw new IgniteException(new NodeStoppingException());
- }
+ return inBusyLockAsync(busyLock, () -> {
+ HybridTimestamp now = clock.now();
- try {
- return supplyAsync(() -> inBusyLock(busyLock, () ->
directTableId(name)), ioExecutor)
- .thenCompose(tableId -> inBusyLock(busyLock, () -> {
- if (tableId == null) {
+ return anyOf(schemaSyncService.waitForMetadataCompleteness(now),
stopManagerFuture)
+ .thenComposeAsync(unused -> inBusyLockAsync(busyLock, ()
-> {
+ CatalogTableDescriptor tableDescriptor =
catalogService.table(name, now.longValue());
+
+ // Check if the table has been deleted.
+ if (tableDescriptor == null) {
return completedFuture(null);
}
- return tableAsyncInternal(tableId, false);
+ return tableAsyncInternalBusy(tableDescriptor.id());
}));
- } finally {
- busyLock.leaveBusy();
- }
+ });
}
- /**
- * Internal method for getting table by id.
- *
- * @param id Table id.
- * @param checkConfiguration {@code True} when the method checks a
configuration before trying to get a table, {@code false}
- * otherwise.
- * @return Future representing pending completion of the operation.
- */
- public CompletableFuture<TableImpl> tableAsyncInternal(int id, boolean
checkConfiguration) {
- CompletableFuture<Boolean> tblCfgFut = checkConfiguration
- ? supplyAsync(() -> inBusyLock(busyLock, () ->
isTableConfigured(id)), ioExecutor)
- : completedFuture(true);
-
- return tblCfgFut.thenCompose(isCfg -> inBusyLock(busyLock, () -> {
- if (!isCfg) {
- return completedFuture(null);
- }
-
- TableImpl tbl = latestTablesById().get(id);
+ private CompletableFuture<TableImpl> tableAsyncInternalBusy(int tableId) {
+ TableImpl tableImpl = latestTablesById().get(tableId);
- if (tbl != null) {
- return completedFuture(tbl);
- }
+ if (tableImpl != null) {
+ return completedFuture(tableImpl);
+ }
- CompletableFuture<TableImpl> getTblFut = new CompletableFuture<>();
+ CompletableFuture<TableImpl> getLatestTableFuture = new
CompletableFuture<>();
- CompletionListener<Void> tablesListener = (token, v, th) -> {
- if (th == null) {
- CompletableFuture<Map<Integer, TableImpl>> tablesFut =
tablesByIdVv.get(token);
+ CompletionListener<Void> tablesListener = (token, v, th) -> {
+ if (th == null) {
+ CompletableFuture<Map<Integer, TableImpl>> tablesFuture =
tablesByIdVv.get(token);
- tablesFut.whenComplete((tables, e) -> {
- if (e != null) {
- getTblFut.completeExceptionally(e);
- } else {
- TableImpl table = tables.get(id);
+ tablesFuture.whenComplete((tables, e) -> {
+ if (e != null) {
+ getLatestTableFuture.completeExceptionally(e);
+ } else {
+ TableImpl table = tables.get(tableId);
- if (table != null) {
- getTblFut.complete(table);
- }
+ if (table != null) {
+ getLatestTableFuture.complete(table);
}
- });
- } else {
- getTblFut.completeExceptionally(th);
- }
- };
-
- assignmentsUpdatedVv.whenComplete(tablesListener);
+ }
+ });
+ } else {
+ getLatestTableFuture.completeExceptionally(th);
+ }
+ };
- // This check is needed for the case when we have registered
tablesListener,
- // but tablesByIdVv has already been completed, so listener would
be triggered only for the next versioned value update.
- tbl = latestTablesById().get(id);
+ assignmentsUpdatedVv.whenComplete(tablesListener);
- if (tbl != null) {
- assignmentsUpdatedVv.removeWhenComplete(tablesListener);
+ // This check is needed for the case when we have registered
tablesListener,
+ // but tablesByIdVv has already been completed, so listener would be
triggered only for the next versioned value update.
+ tableImpl = latestTablesById().get(tableId);
- return completedFuture(tbl);
- }
+ if (tableImpl != null) {
+ assignmentsUpdatedVv.removeWhenComplete(tablesListener);
- return getTblFut.whenComplete((unused, throwable) ->
assignmentsUpdatedVv.removeWhenComplete(tablesListener));
- }));
- }
+ return completedFuture(tableImpl);
+ }
- /**
- * Checks that the table is configured with specific id.
- *
- * @param id Table id.
- * @return True when the table is configured into cluster, false otherwise.
- */
- private boolean isTableConfigured(int id) {
- return configuredTablesCache.isTableConfigured(id);
+ return anyOf(getLatestTableFuture, stopManagerFuture)
+ .thenComposeAsync(o -> getLatestTableFuture, ioExecutor)
Review Comment:
what's the point of using `thenComposeAsync` instead of `thenCompose` here?
Also, what's the point of `anyOf` above, which uses the same future, if we
still need to wait for it?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -240,8 +237,11 @@ public class PartitionReplicaListener implements
ReplicaListener {
private final CatalogService catalogService;
- /** Listener for configuration indexes, {@code null} if the replica is not
the leader. */
- private final
AtomicReference<ConfigurationNamedListListener<TableIndexView>>
indexesConfigurationListener = new AtomicReference<>();
+ /** Listener for creating an index in catalog, {@code null} if the replica
is not the leader. */
+ private final AtomicReference<EventListener<CreateIndexEventParameters>>
createIndexListener = new AtomicReference<>();
Review Comment:
Do we need to use `AtomicReferenceFieldUpdater` here?
`PartitionReplicaListener` is created for every partition
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
