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