You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sashapolo (via GitHub)" <gi...@apache.org> on 2023/06/07 04:43:25 UTC
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #2143: IGNITE-19646 Transform IndexManager to internally work against Catalog event types
sashapolo commented on code in PR #2143:
URL: https://github.com/apache/ignite-3/pull/2143#discussion_r1220091157
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java:
##########
@@ -436,31 +436,39 @@ private CompletableFuture<?> onIndexCreate(ConfigurationNotificationEvent<TableI
}
try {
- return createIndexLocally(evt.storageRevision(), tableId, evt.newValue(), evt.newValue(TablesView.class));
+ TablesView tablesView = evt.newValue(TablesView.class);
+
+ TableView tableView = findTableView(tableId, (NamedListView<TableView>) tablesView.tables());
+
+ org.apache.ignite.internal.catalog.descriptors.TableDescriptor catalogTableDescriptor = toTableDescriptor(tableView);
+ org.apache.ignite.internal.catalog.descriptors.IndexDescriptor catalogIndexDescriptor = toIndexDescriptor(indexConfig);
+
+ return createIndexLocally(evt.storageRevision(), catalogTableDescriptor, catalogIndexDescriptor);
} finally {
busyLock.leaveBusy();
}
}
private CompletableFuture<?> createIndexLocally(
long causalityToken,
- int tableId,
- TableIndexView tableIndexView,
- TablesView tablesView
+ org.apache.ignite.internal.catalog.descriptors.TableDescriptor catalogTableDescriptor,
Review Comment:
Why do you call these parameters `catalog*`, isn't that obvious from their type?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ClusterPerClassIntegrationTest.java:
##########
@@ -574,4 +585,15 @@ public static TablesConfiguration getTablesConfiguration(Ignite node) {
return indexConfig == null ? null : indexConfig.id().value();
}
+
+ /**
+ * Returns table configuration of the given table at the given node, or {@code null} if no such table exists.
+ *
+ * @param node Node.
+ * @param tableName Table name.
+ * @return Table configuration.
+ */
+ public static @Nullable TableConfiguration getTableConfiguration(Ignite node, String tableName) {
Review Comment:
can be private
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java:
##########
@@ -436,31 +436,39 @@ private CompletableFuture<?> onIndexCreate(ConfigurationNotificationEvent<TableI
}
try {
- return createIndexLocally(evt.storageRevision(), tableId, evt.newValue(), evt.newValue(TablesView.class));
+ TablesView tablesView = evt.newValue(TablesView.class);
+
+ TableView tableView = findTableView(tableId, (NamedListView<TableView>) tablesView.tables());
+
+ org.apache.ignite.internal.catalog.descriptors.TableDescriptor catalogTableDescriptor = toTableDescriptor(tableView);
Review Comment:
IDEA tells me that `TableDescriptor` can still be imported. Do you use the fully qualified name on purpose?
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java:
##########
@@ -436,31 +436,39 @@ private CompletableFuture<?> onIndexCreate(ConfigurationNotificationEvent<TableI
}
try {
- return createIndexLocally(evt.storageRevision(), tableId, evt.newValue(), evt.newValue(TablesView.class));
+ TablesView tablesView = evt.newValue(TablesView.class);
+
+ TableView tableView = findTableView(tableId, (NamedListView<TableView>) tablesView.tables());
Review Comment:
`findTableView` can return `null`, looks like you do not handle this situation
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexUtils.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.index;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Helper class for working with indexes.
+ */
+class IndexUtils {
Review Comment:
What's the point of this class? It's only used in the `IndexManager`, can we move these methods there?
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java:
##########
@@ -436,31 +436,39 @@ private CompletableFuture<?> onIndexCreate(ConfigurationNotificationEvent<TableI
}
try {
- return createIndexLocally(evt.storageRevision(), tableId, evt.newValue(), evt.newValue(TablesView.class));
+ TablesView tablesView = evt.newValue(TablesView.class);
+
+ TableView tableView = findTableView(tableId, (NamedListView<TableView>) tablesView.tables());
+
+ org.apache.ignite.internal.catalog.descriptors.TableDescriptor catalogTableDescriptor = toTableDescriptor(tableView);
+ org.apache.ignite.internal.catalog.descriptors.IndexDescriptor catalogIndexDescriptor = toIndexDescriptor(indexConfig);
+
+ return createIndexLocally(evt.storageRevision(), catalogTableDescriptor, catalogIndexDescriptor);
} finally {
busyLock.leaveBusy();
}
}
private CompletableFuture<?> createIndexLocally(
long causalityToken,
- int tableId,
- TableIndexView tableIndexView,
- TablesView tablesView
+ org.apache.ignite.internal.catalog.descriptors.TableDescriptor catalogTableDescriptor,
+ org.apache.ignite.internal.catalog.descriptors.IndexDescriptor catalogIndexDescriptor
) {
- assert tableIndexView != null;
-
- int indexId = tableIndexView.id();
+ int tableId = catalogTableDescriptor.id();
+ int indexId = catalogIndexDescriptor.id();
LOG.trace("Creating local index: name={}, id={}, tableId={}, token={}",
- tableIndexView.name(), indexId, tableId, causalityToken);
+ catalogIndexDescriptor.name(), indexId, tableId, causalityToken);
- IndexDescriptor descriptor = newDescriptor(tableIndexView);
+ IndexDescriptor eventIndexDescriptor = toEventIndexDescriptor(catalogIndexDescriptor);
Review Comment:
Jesus, do we have 3 `IndexDescriptor` types which are all used here? Can we at least get rid of this one?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java:
##########
@@ -765,4 +781,18 @@ private List<RocksDbSortedIndexStorage> getSortedIndexStorages(int partitionId)
return (IndexStorage) null;
});
}
+
+ private static @Nullable TableIndexView findIndexView(TablesView tablesView, int indexId) {
Review Comment:
This stuff is copy-pasted everywhere, can it be extracted somewhere?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/IndexDescriptor.java:
##########
@@ -59,23 +55,23 @@ interface ColumnDescriptor {
List<? extends ColumnDescriptor> columns();
/**
- * Creates an index description based on the configuration.
+ * Creates an index description based on the catalog descriptors.
*
- * @param tablesView Tables configuration.
- * @param indexId Index ID.
+ * @param table Catalog table descriptor.
+ * @param index Catalog index descriptor.
*/
- static IndexDescriptor createIndexDescriptor(TablesView tablesView, int indexId) {
- TableIndexView indexView = tablesView.indexes().stream()
- .filter(tableIndexView -> indexId == tableIndexView.id())
- .findFirst()
- .orElse(null);
+ static IndexDescriptor createIndexDescriptor(
Review Comment:
`IndexDescriptor.createIndexDescriptor` is a bit of a mouthful, I think it can simply be called `IndexDescriptor.create`
--
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