You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "tkalkirill (via GitHub)" <gi...@apache.org> on 2023/06/21 11:41:12 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request, #2231: IGNITE-19641 Catalog events are triggered too early.
tkalkirill opened a new pull request, #2231:
URL: https://github.com/apache/ignite-3/pull/2231
https://issues.apache.org/jira/browse/IGNITE-19641
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237557975
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AddColumnEventParameters.java:
##########
@@ -24,25 +24,27 @@
* Add column event parameters contains descriptors of added columns.
*/
public class AddColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final List<CatalogTableColumnDescriptor> columnDescriptors;
+ private final List<CatalogTableColumnDescriptor> descriptors;
Review Comment:
But not for me.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AlterColumnEventParameters.java:
##########
@@ -23,32 +23,36 @@
* Create table event parameters contains a column descriptor for the modified column.
*/
public class AlterColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final CatalogTableColumnDescriptor columnDescriptor;
+ private final CatalogTableColumnDescriptor descriptor;
Review Comment:
But not for me.
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238050421
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/CatalogFireEvent.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.storage;
+
+import org.apache.ignite.internal.catalog.events.CatalogEvent;
+import org.apache.ignite.internal.catalog.events.CatalogEventParameters;
+
+/**
+ * Interface for updates that require firing events.
+ */
+public interface CatalogFireEvent {
Review Comment:
Within the package 'storage' there is no single class whose name has prefix "Catalog", thus let's make naming consistent.
Also, `fireEvent` is a good name for method to fire event, but not for the object that provides the event. Have you considered the names like `Fireable`, `EventSource`, or `EventProvider`?
--
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
[GitHub] [ignite-3] tkalkirill merged pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237119534
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -277,82 +259,62 @@ private Catalog catalogAt(long timestamp) {
return entry.getValue();
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> createTable(CreateTableParams params) {
return saveUpdate(catalog -> {
- String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
-
- CatalogSchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+ CatalogSchemaDescriptor schema = getSchema(catalog, params.schemaName());
if (schema.table(params.tableName()) != null) {
- throw new TableAlreadyExistsException(schemaName, params.tableName());
+ throw new TableAlreadyExistsException(schema.name(), params.tableName());
}
- params.columns().stream().map(ColumnParams::name).filter(Predicate.not(new HashSet<>()::add))
- .findAny().ifPresent(columnName -> {
- throw new IgniteInternalException(
- ErrorGroups.Index.INVALID_INDEX_DEFINITION_ERR, "Can't create table with duplicate columns: "
- + params.columns().stream().map(ColumnParams::name).collect(Collectors.joining(", "))
- );
- });
+ validateParams(params);
+
+ CatalogZoneDescriptor zone = getZone(catalog, Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
- String zoneName = Objects.requireNonNullElse(params.zone(), CatalogService.DEFAULT_ZONE_NAME);
+ int id = catalog.objectIdGenState();
- CatalogZoneDescriptor zone = Objects.requireNonNull(catalog.zone(zoneName), "No zone found: " + zoneName);
+ CatalogTableDescriptor table = CatalogUtils.fromParams(id++, zone.id(), params);
- CatalogTableDescriptor table = CatalogUtils.fromParams(catalog.objectIdGenState(), zone.id(), params);
+ CatalogHashIndexDescriptor pkIndex = createHashIndexDescriptor(createPkIndexParams(params), table, id++);
return List.of(
new NewTableEntry(table),
- new ObjectIdGenUpdateEntry(1)
+ new NewIndexEntry(pkIndex),
+ new ObjectIdGenUpdateEntry(id - catalog.objectIdGenState())
);
});
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropTable(DropTableParams params) {
return saveUpdate(catalog -> {
- String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
-
- CatalogSchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+ CatalogSchemaDescriptor schema = getSchema(catalog, params.schemaName());
- CatalogTableDescriptor table = schema.table(params.tableName());
-
- if (table == null) {
- throw new TableNotFoundException(schemaName, params.tableName());
- }
+ CatalogTableDescriptor table = getTable(schema, params.tableName());
List<UpdateEntry> updateEntries = new ArrayList<>();
Arrays.stream(schema.indexes())
.filter(index -> index.tableId() == table.id())
- .forEach(index -> updateEntries.add(new DropIndexEntry(index.id())));
+ .forEach(index -> updateEntries.add(new DropIndexEntry(index.id(), index.tableId())));
Review Comment:
Why do you need a tableId here?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AbstractIndexCommandParams.java:
##########
@@ -41,6 +47,20 @@ public String schemaName() {
return schema;
}
+ /**
+ * Gets table name.
+ */
+ public String tableName() {
+ return tableName;
+ }
+
+ /**
+ * Returns {@code true} if index is unique, {@code false} otherwise.
+ */
+ public boolean unique() {
Review Comment:
What does this have to do with current fix?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -277,82 +259,62 @@ private Catalog catalogAt(long timestamp) {
return entry.getValue();
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> createTable(CreateTableParams params) {
return saveUpdate(catalog -> {
- String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
-
- CatalogSchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+ CatalogSchemaDescriptor schema = getSchema(catalog, params.schemaName());
if (schema.table(params.tableName()) != null) {
- throw new TableAlreadyExistsException(schemaName, params.tableName());
+ throw new TableAlreadyExistsException(schema.name(), params.tableName());
}
- params.columns().stream().map(ColumnParams::name).filter(Predicate.not(new HashSet<>()::add))
- .findAny().ifPresent(columnName -> {
- throw new IgniteInternalException(
- ErrorGroups.Index.INVALID_INDEX_DEFINITION_ERR, "Can't create table with duplicate columns: "
- + params.columns().stream().map(ColumnParams::name).collect(Collectors.joining(", "))
- );
- });
+ validateParams(params);
+
+ CatalogZoneDescriptor zone = getZone(catalog, Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
Review Comment:
This and similar refactorings could have been done in another PR, they have nothing to do with the fix itself.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AddColumnEventParameters.java:
##########
@@ -24,25 +24,27 @@
* Add column event parameters contains descriptors of added columns.
*/
public class AddColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final List<CatalogTableColumnDescriptor> columnDescriptors;
+ private final List<CatalogTableColumnDescriptor> descriptors;
Review Comment:
Why did you rename it? Old name was fine
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AlterZoneEventParameters.java:
##########
@@ -23,25 +23,25 @@
* Alter zone event parameters contains a distribution zone descriptor for newly created distribution zone.
*/
public class AlterZoneEventParameters extends CatalogEventParameters {
-
- private final CatalogZoneDescriptor zoneDescriptor;
+ private final CatalogZoneDescriptor descriptor;
Review Comment:
Same here. Why did you rename all these fields?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AlterColumnEventParameters.java:
##########
@@ -23,32 +23,36 @@
* Create table event parameters contains a column descriptor for the modified column.
*/
public class AlterColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final CatalogTableColumnDescriptor columnDescriptor;
+ private final CatalogTableColumnDescriptor descriptor;
/**
* Constructor.
*
* @param causalityToken Causality token.
- * @param tableId Returns an id the table to be modified.
- * @param columnDescriptor Descriptor for the column to be replaced.
+ * @param catalogVersion Catalog version.
+ * @param tableId Returns ID the table to be modified.
+ * @param descriptor Descriptor for the column to be replaced.
*/
- public AlterColumnEventParameters(long causalityToken, int tableId, CatalogTableColumnDescriptor columnDescriptor) {
- super(causalityToken);
+ public AlterColumnEventParameters(long causalityToken, int catalogVersion, int tableId, CatalogTableColumnDescriptor descriptor) {
+ super(causalityToken, catalogVersion);
this.tableId = tableId;
- this.columnDescriptor = columnDescriptor;
+ this.descriptor = descriptor;
}
- /** Returns an id of a modified table. */
+ /**
+ * Returns ID of a modified table.
+ */
Review Comment:
Was this necessary? Do we have code style guide that recommends this formatting?
If not, then you simply change someone-else's code by your own preference, without any other objective reason.
I wouldn't mind it, but this PR is 1000+ lines of changes, and some of them are completely unnecessary. This forces me to look at more code than I should.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AlterColumnEventParameters.java:
##########
@@ -23,32 +23,36 @@
* Create table event parameters contains a column descriptor for the modified column.
*/
public class AlterColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final CatalogTableColumnDescriptor columnDescriptor;
+ private final CatalogTableColumnDescriptor descriptor;
Review Comment:
And here. These are changes for the sake of changes
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateLogImpl.java:
##########
@@ -167,7 +169,7 @@ private void restoreStateFromVault(OnUpdateHandler handler) {
VersionedUpdate update = fromBytes(entry.value());
- handler.handle(update);
+ handler.handle(update, metastore.appliedRevision());
Review Comment:
This has nothing to do with the fix. Please do it in a separate issue.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/CatalogFireEvent.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.storage;
+
+import org.apache.ignite.internal.catalog.events.CatalogEvent;
+import org.apache.ignite.internal.catalog.events.CatalogEventParameters;
+
+/**
+ * Interface for firing events.
Review Comment:
for updates that require firing events, I guess
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/AlterZoneEntry.java:
##########
@@ -37,12 +43,36 @@ public AlterZoneEntry(CatalogZoneDescriptor descriptor) {
this.descriptor = descriptor;
}
- /** Returns descriptor of a zone to alter. */
+ /**
+ * Returns descriptor of a zone to alter.
+ */
public CatalogZoneDescriptor descriptor() {
return descriptor;
}
- /** {@inheritDoc} */
+ @Override
+ public CatalogEvent eventType() {
+ return CatalogEvent.ZONE_ALTER;
+ }
+
+ @Override
+ public CatalogEventParameters createEventParameters(long causalityToken, int catalogVersion) {
+ return new AlterZoneEventParameters(causalityToken, catalogVersion, descriptor);
+ }
+
+ @Override
+ public Catalog applyUpdate(Catalog catalog, VersionedUpdate update) {
+ return new Catalog(
+ update.version(),
+ update.activationTimestamp(),
+ catalog.objectIdGenState(),
+ catalog.zones().stream()
+ .map(z -> z.id() == descriptor.id() ? descriptor : z)
Review Comment:
`z`, is this the original name?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AlterZoneEventParameters.java:
##########
@@ -23,25 +23,25 @@
* Alter zone event parameters contains a distribution zone descriptor for newly created distribution zone.
*/
public class AlterZoneEventParameters extends CatalogEventParameters {
-
- private final CatalogZoneDescriptor zoneDescriptor;
+ private final CatalogZoneDescriptor descriptor;
Review Comment:
I won't be leaving the same comment over and over again. I see these renames multiple times later.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateEntry.java:
##########
@@ -18,9 +18,11 @@
package org.apache.ignite.internal.catalog.storage;
import java.io.Serializable;
+import org.apache.ignite.internal.catalog.Catalog;
/**
- * A marker interface describing a particular change within the {@link VersionedUpdate group}.
+ * Interface describing a particular change within the {@link VersionedUpdate group}.
*/
public interface UpdateEntry extends Serializable {
+ Catalog applyUpdate(Catalog catalog, VersionedUpdate update);
Review Comment:
Where's javadoc?
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238615534
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateEntry.java:
##########
@@ -18,9 +18,18 @@
package org.apache.ignite.internal.catalog.storage;
import java.io.Serializable;
+import org.apache.ignite.internal.catalog.Catalog;
/**
- * A marker interface describing a particular change within the {@link VersionedUpdate group}.
+ * Interface describing a particular change within the {@link VersionedUpdate group}.
*/
public interface UpdateEntry extends Serializable {
+ /**
+ * Applies own change to the catalog.
+ *
+ * @param catalog Current catalog.
+ * @param update Group of changes that relates to specified version.
+ * @return New catalog.
+ */
+ Catalog applyUpdate(Catalog catalog, VersionedUpdate update);
Review Comment:
this signature doesn't feel right. Which update should I apply to the catalog? Why do I need VersionedUpdate at all? Should I verify that current entry is part of VersionedUpdate.
If you need to bump version, it's better to do as post processing of all updates
--
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
[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237074963
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -651,18 +456,12 @@ public CompletableFuture<Void> createDistributionZone(CreateZoneParams params) {
});
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropDistributionZone(DropZoneParams params) {
return saveUpdate(catalog -> {
- String zoneName = Objects.requireNonNull(params.zoneName(), "zone");
+ CatalogZoneDescriptor zone = getZone(catalog, params.zoneName());
- CatalogZoneDescriptor zone = catalog.zone(zoneName);
-
- if (zone == null) {
- throw new DistributionZoneNotFoundException(zoneName);
- }
- if (zone.name().equals(CatalogService.DEFAULT_ZONE_NAME)) {
+ if (zone.name().equals(DEFAULT_ZONE_NAME)) {
//TODO IGNITE-19082 Can default zone be dropped?
Review Comment:
Default zone can't be dropped. Later, we may allow to change default zone
https://issues.apache.org/jira/browse/IGNITE-19687
Let's just drop the todo.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237456344
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -651,18 +456,12 @@ public CompletableFuture<Void> createDistributionZone(CreateZoneParams params) {
});
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropDistributionZone(DropZoneParams params) {
return saveUpdate(catalog -> {
- String zoneName = Objects.requireNonNull(params.zoneName(), "zone");
+ CatalogZoneDescriptor zone = getZone(catalog, params.zoneName());
- CatalogZoneDescriptor zone = catalog.zone(zoneName);
-
- if (zone == null) {
- throw new DistributionZoneNotFoundException(zoneName);
- }
- if (zone.name().equals(CatalogService.DEFAULT_ZONE_NAME)) {
+ if (zone.name().equals(DEFAULT_ZONE_NAME)) {
//TODO IGNITE-19082 Can default zone be dropped?
Review Comment:
Fix it
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238093227
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -36,7 +36,7 @@
* <p>TBD: events
*/
public interface CatalogService {
- String PUBLIC = "PUBLIC";
Review Comment:
I would say that this remove is justified, but in another PR maybe. Name "PUBLIC" says nothing about what it is, and there's no javadoc to clarify things either.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237515837
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -277,82 +259,62 @@ private Catalog catalogAt(long timestamp) {
return entry.getValue();
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> createTable(CreateTableParams params) {
return saveUpdate(catalog -> {
- String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
-
- CatalogSchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+ CatalogSchemaDescriptor schema = getSchema(catalog, params.schemaName());
if (schema.table(params.tableName()) != null) {
- throw new TableAlreadyExistsException(schemaName, params.tableName());
+ throw new TableAlreadyExistsException(schema.name(), params.tableName());
}
- params.columns().stream().map(ColumnParams::name).filter(Predicate.not(new HashSet<>()::add))
- .findAny().ifPresent(columnName -> {
- throw new IgniteInternalException(
- ErrorGroups.Index.INVALID_INDEX_DEFINITION_ERR, "Can't create table with duplicate columns: "
- + params.columns().stream().map(ColumnParams::name).collect(Collectors.joining(", "))
- );
- });
+ validateParams(params);
+
+ CatalogZoneDescriptor zone = getZone(catalog, Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
- String zoneName = Objects.requireNonNullElse(params.zone(), CatalogService.DEFAULT_ZONE_NAME);
+ int id = catalog.objectIdGenState();
- CatalogZoneDescriptor zone = Objects.requireNonNull(catalog.zone(zoneName), "No zone found: " + zoneName);
+ CatalogTableDescriptor table = CatalogUtils.fromParams(id++, zone.id(), params);
- CatalogTableDescriptor table = CatalogUtils.fromParams(catalog.objectIdGenState(), zone.id(), params);
+ CatalogHashIndexDescriptor pkIndex = createHashIndexDescriptor(createPkIndexParams(params), table, id++);
return List.of(
new NewTableEntry(table),
- new ObjectIdGenUpdateEntry(1)
+ new NewIndexEntry(pkIndex),
+ new ObjectIdGenUpdateEntry(id - catalog.objectIdGenState())
);
});
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropTable(DropTableParams params) {
return saveUpdate(catalog -> {
- String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
-
- CatalogSchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+ CatalogSchemaDescriptor schema = getSchema(catalog, params.schemaName());
- CatalogTableDescriptor table = schema.table(params.tableName());
-
- if (table == null) {
- throw new TableNotFoundException(schemaName, params.tableName());
- }
+ CatalogTableDescriptor table = getTable(schema, params.tableName());
List<UpdateEntry> updateEntries = new ArrayList<>();
Arrays.stream(schema.indexes())
.filter(index -> index.tableId() == table.id())
- .forEach(index -> updateEntries.add(new DropIndexEntry(index.id())));
+ .forEach(index -> updateEntries.add(new DropIndexEntry(index.id(), index.tableId())));
Review Comment:
Made a separate ticket for this https://issues.apache.org/jira/browse/IGNITE-19798
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238047928
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1248,19 +1275,22 @@ public void testCreateIndexEvents() {
// Create index.
assertThat(service.createIndex(createIndexParams), willCompleteSuccessfully());
- verify(eventListener).notify(any(CreateIndexEventParameters.class), ArgumentMatchers.isNull());
+ verify(eventListener, times(2)).notify(any(CreateIndexEventParameters.class), isNull());
// Drop index.
assertThat(service.dropIndex(dropIndexParams), willCompleteSuccessfully());
- verify(eventListener).notify(any(DropIndexEventParameters.class), ArgumentMatchers.isNull());
+ verify(eventListener).notify(any(DropIndexEventParameters.class), isNull());
+
+ clearInvocations(eventListener);
// Drop table.
assertThat(service.dropTable(dropTableparams), willCompleteSuccessfully());
// Try drop index once again.
assertThat(service.dropIndex(dropIndexParams), willThrow(IndexNotFoundException.class));
- verifyNoMoreInteractions(eventListener);
+ // Should have destroyed the pk index.
+ verify(eventListener).notify(any(DropIndexEventParameters.class), isNull());
Review Comment:
Revert it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237551575
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AbstractIndexCommandParams.java:
##########
@@ -41,6 +47,20 @@ public String schemaName() {
return schema;
}
+ /**
+ * Gets table name.
+ */
+ public String tableName() {
+ return tableName;
+ }
+
+ /**
+ * Returns {@code true} if index is unique, {@code false} otherwise.
+ */
+ public boolean unique() {
Review Comment:
Create ticket for this https://issues.apache.org/jira/browse/IGNITE-19799
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238089630
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AlterColumnEventParameters.java:
##########
@@ -23,32 +23,36 @@
* Create table event parameters contains a column descriptor for the modified column.
*/
public class AlterColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final CatalogTableColumnDescriptor columnDescriptor;
+ private final CatalogTableColumnDescriptor descriptor;
/**
* Constructor.
*
* @param causalityToken Causality token.
- * @param tableId Returns an id the table to be modified.
- * @param columnDescriptor Descriptor for the column to be replaced.
+ * @param catalogVersion Catalog version.
+ * @param tableId Returns ID the table to be modified.
+ * @param descriptor Descriptor for the column to be replaced.
*/
- public AlterColumnEventParameters(long causalityToken, int tableId, CatalogTableColumnDescriptor columnDescriptor) {
- super(causalityToken);
+ public AlterColumnEventParameters(long causalityToken, int catalogVersion, int tableId, CatalogTableColumnDescriptor descriptor) {
+ super(causalityToken, catalogVersion);
this.tableId = tableId;
- this.columnDescriptor = columnDescriptor;
+ this.descriptor = descriptor;
}
- /** Returns an id of a modified table. */
+ /**
+ * Returns ID of a modified table.
+ */
Review Comment:
Ok, but converting
```
/** Foo. */
```
into
```
/**
* Foo.
*/
```
is not related to grammar
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1239353061
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateEntry.java:
##########
@@ -18,9 +18,18 @@
package org.apache.ignite.internal.catalog.storage;
import java.io.Serializable;
+import org.apache.ignite.internal.catalog.Catalog;
/**
- * A marker interface describing a particular change within the {@link VersionedUpdate group}.
+ * Interface describing a particular change within the {@link VersionedUpdate group}.
*/
public interface UpdateEntry extends Serializable {
+ /**
+ * Applies own change to the catalog.
+ *
+ * @param catalog Current catalog.
+ * @param update Group of changes that relates to specified version.
+ * @return New catalog.
+ */
+ Catalog applyUpdate(Catalog catalog, VersionedUpdate update);
Review Comment:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237464401
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1248,19 +1275,22 @@ public void testCreateIndexEvents() {
// Create index.
assertThat(service.createIndex(createIndexParams), willCompleteSuccessfully());
- verify(eventListener).notify(any(CreateIndexEventParameters.class), ArgumentMatchers.isNull());
+ verify(eventListener, times(2)).notify(any(CreateIndexEventParameters.class), isNull());
// Drop index.
assertThat(service.dropIndex(dropIndexParams), willCompleteSuccessfully());
- verify(eventListener).notify(any(DropIndexEventParameters.class), ArgumentMatchers.isNull());
+ verify(eventListener).notify(any(DropIndexEventParameters.class), isNull());
+
+ clearInvocations(eventListener);
// Drop table.
assertThat(service.dropTable(dropTableparams), willCompleteSuccessfully());
// Try drop index once again.
assertThat(service.dropIndex(dropIndexParams), willThrow(IndexNotFoundException.class));
- verifyNoMoreInteractions(eventListener);
+ // Should have destroyed the pk index.
+ verify(eventListener).notify(any(DropIndexEventParameters.class), isNull());
Review Comment:
Before dropping the table, I call `clearInvocations(eventListener);`
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238021635
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -36,7 +36,7 @@
* <p>TBD: events
*/
public interface CatalogService {
- String PUBLIC = "PUBLIC";
Review Comment:
was it really necessary to rename this constant?
--
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
[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237108035
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1248,19 +1275,22 @@ public void testCreateIndexEvents() {
// Create index.
assertThat(service.createIndex(createIndexParams), willCompleteSuccessfully());
- verify(eventListener).notify(any(CreateIndexEventParameters.class), ArgumentMatchers.isNull());
+ verify(eventListener, times(2)).notify(any(CreateIndexEventParameters.class), isNull());
// Drop index.
assertThat(service.dropIndex(dropIndexParams), willCompleteSuccessfully());
- verify(eventListener).notify(any(DropIndexEventParameters.class), ArgumentMatchers.isNull());
+ verify(eventListener).notify(any(DropIndexEventParameters.class), isNull());
+
+ clearInvocations(eventListener);
// Drop table.
assertThat(service.dropTable(dropTableparams), willCompleteSuccessfully());
// Try drop index once again.
assertThat(service.dropIndex(dropIndexParams), willThrow(IndexNotFoundException.class));
- verifyNoMoreInteractions(eventListener);
+ // Should have destroyed the pk index.
+ verify(eventListener).notify(any(DropIndexEventParameters.class), isNull());
Review Comment:
It is not clear, if `DropIndexEventParameters` was caused by `dropTable` or `dropIndex` call.
Let's move this to few lines higher.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237587629
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateLogImpl.java:
##########
@@ -167,7 +169,7 @@ private void restoreStateFromVault(OnUpdateHandler handler) {
VersionedUpdate update = fromBytes(entry.value());
- handler.handle(update);
+ handler.handle(update, metastore.appliedRevision());
Review Comment:
But the catalog creates events that are descendants of `org.apache.ignite.internal.manager.EventParameters` that require `causalityToken`, the catalog now passes the correct value.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238002599
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateEntry.java:
##########
@@ -18,9 +18,11 @@
package org.apache.ignite.internal.catalog.storage;
import java.io.Serializable;
+import org.apache.ignite.internal.catalog.Catalog;
/**
- * A marker interface describing a particular change within the {@link VersionedUpdate group}.
+ * Interface describing a particular change within the {@link VersionedUpdate group}.
*/
public interface UpdateEntry extends Serializable {
+ Catalog applyUpdate(Catalog catalog, VersionedUpdate update);
Review Comment:
Trie to fix it
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238017426
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AddColumnEventParameters.java:
##########
@@ -24,25 +24,27 @@
* Add column event parameters contains descriptors of added columns.
*/
public class AddColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final List<CatalogTableColumnDescriptor> columnDescriptors;
+ private final List<CatalogTableColumnDescriptor> descriptors;
Review Comment:
what is wrong with original name?
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238025041
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -277,29 +254,18 @@ private Catalog catalogAt(long timestamp) {
return entry.getValue();
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> createTable(CreateTableParams params) {
return saveUpdate(catalog -> {
- String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
-
- CatalogSchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+ CatalogSchemaDescriptor schema = getSchema(catalog, params.schemaName());
if (schema.table(params.tableName()) != null) {
- throw new TableAlreadyExistsException(schemaName, params.tableName());
+ throw new TableAlreadyExistsException(schema.name(), params.tableName());
}
- params.columns().stream().map(ColumnParams::name).filter(Predicate.not(new HashSet<>()::add))
- .findAny().ifPresent(columnName -> {
- throw new IgniteInternalException(
- ErrorGroups.Index.INVALID_INDEX_DEFINITION_ERR, "Can't create table with duplicate columns: "
- + params.columns().stream().map(ColumnParams::name).collect(Collectors.joining(", "))
- );
- });
+ validateParams(params);
- String zoneName = Objects.requireNonNullElse(params.zone(), CatalogService.DEFAULT_ZONE_NAME);
-
- CatalogZoneDescriptor zone = Objects.requireNonNull(catalog.zone(zoneName), "No zone found: " + zoneName);
+ CatalogZoneDescriptor zone = getZone(catalog, Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
Review Comment:
All this refactoring has nothing to do with problem to be solved within the ticket (IGNITE-19641). So, please, move it to another patch
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238031732
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/CatalogEventParameters.java:
##########
@@ -23,12 +23,24 @@
* Base class for Catalog event parameters.
*/
public abstract class CatalogEventParameters extends EventParameters {
+ private final int catalogVersion;
+
/**
* Constructor.
*
* @param causalityToken Causality token.
+ * @param catalogVersion Catalog version.
*/
- public CatalogEventParameters(long causalityToken) {
+ public CatalogEventParameters(long causalityToken, int catalogVersion) {
Review Comment:
why version of the catalog can't be causality token?
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238092368
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateLogImpl.java:
##########
@@ -167,7 +169,7 @@ private void restoreStateFromVault(OnUpdateHandler handler) {
VersionedUpdate update = fromBytes(entry.value());
- handler.handle(update);
+ handler.handle(update, metastore.appliedRevision());
Review Comment:
It's only required now because you added it, but ok, this change is not as big as the rest
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237469616
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -277,82 +259,62 @@ private Catalog catalogAt(long timestamp) {
return entry.getValue();
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> createTable(CreateTableParams params) {
return saveUpdate(catalog -> {
- String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
-
- CatalogSchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+ CatalogSchemaDescriptor schema = getSchema(catalog, params.schemaName());
if (schema.table(params.tableName()) != null) {
- throw new TableAlreadyExistsException(schemaName, params.tableName());
+ throw new TableAlreadyExistsException(schema.name(), params.tableName());
}
- params.columns().stream().map(ColumnParams::name).filter(Predicate.not(new HashSet<>()::add))
- .findAny().ifPresent(columnName -> {
- throw new IgniteInternalException(
- ErrorGroups.Index.INVALID_INDEX_DEFINITION_ERR, "Can't create table with duplicate columns: "
- + params.columns().stream().map(ColumnParams::name).collect(Collectors.joining(", "))
- );
- });
+ validateParams(params);
+
+ CatalogZoneDescriptor zone = getZone(catalog, Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
Review Comment:
What exactly should be done in another ticket I don't understand.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237583162
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/CatalogFireEvent.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.storage;
+
+import org.apache.ignite.internal.catalog.events.CatalogEvent;
+import org.apache.ignite.internal.catalog.events.CatalogEventParameters;
+
+/**
+ * Interface for firing events.
Review Comment:
fix it
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238084242
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/CatalogEventParameters.java:
##########
@@ -23,12 +23,24 @@
* Base class for Catalog event parameters.
*/
public abstract class CatalogEventParameters extends EventParameters {
+ private final int catalogVersion;
+
/**
* Constructor.
*
* @param causalityToken Causality token.
+ * @param catalogVersion Catalog version.
*/
- public CatalogEventParameters(long causalityToken) {
+ public CatalogEventParameters(long causalityToken, int catalogVersion) {
Review Comment:
I was involved in it. Right now we only have meta-storage revision as a causality token. If we use two different entities, calling it the same name, it will lead to confusion. It's easier and safer to migrate meta-storage revision into a catalog version later with a single commit
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238085646
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AlterColumnEventParameters.java:
##########
@@ -23,32 +23,36 @@
* Create table event parameters contains a column descriptor for the modified column.
*/
public class AlterColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final CatalogTableColumnDescriptor columnDescriptor;
+ private final CatalogTableColumnDescriptor descriptor;
Review Comment:
Please justify it then, providing a good argument.
Again, this would be fine if you didn't have another 1000 lines of refactoring 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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238029989
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -1089,4 +644,195 @@ private static void throwUnsupportedDdl(String msg, Object... params) {
interface UpdateProducer {
List<UpdateEntry> get(Catalog catalog);
}
+
+ private static CatalogSchemaDescriptor getSchema(Catalog catalog, @Nullable String schemaName) {
Review Comment:
all this new lines have nothing to do with the problem described in ticket, so, please, move it to another patch
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238087675
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -277,82 +259,62 @@ private Catalog catalogAt(long timestamp) {
return entry.getValue();
}
- /** {@inheritDoc} */
@Override
public CompletableFuture<Void> createTable(CreateTableParams params) {
return saveUpdate(catalog -> {
- String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
-
- CatalogSchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+ CatalogSchemaDescriptor schema = getSchema(catalog, params.schemaName());
if (schema.table(params.tableName()) != null) {
- throw new TableAlreadyExistsException(schemaName, params.tableName());
+ throw new TableAlreadyExistsException(schema.name(), params.tableName());
}
- params.columns().stream().map(ColumnParams::name).filter(Predicate.not(new HashSet<>()::add))
- .findAny().ifPresent(columnName -> {
- throw new IgniteInternalException(
- ErrorGroups.Index.INVALID_INDEX_DEFINITION_ERR, "Can't create table with duplicate columns: "
- + params.columns().stream().map(ColumnParams::name).collect(Collectors.joining(", "))
- );
- });
+ validateParams(params);
+
+ CatalogZoneDescriptor zone = getZone(catalog, Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
Review Comment:
Refactoring that has nothing to do with the fix you provide. `createTable` is not related to anything described in the original issue, just as any other "alter" methods. You only changed the part where updates are being converted to events. I'm all for refactoring that part, it's good, but everything else is just confusing and unnecessary
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237563034
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AlterZoneEventParameters.java:
##########
@@ -23,25 +23,25 @@
* Alter zone event parameters contains a distribution zone descriptor for newly created distribution zone.
*/
public class AlterZoneEventParameters extends CatalogEventParameters {
-
- private final CatalogZoneDescriptor zoneDescriptor;
+ private final CatalogZoneDescriptor descriptor;
Review Comment:
But not for me.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/AlterZoneEntry.java:
##########
@@ -37,12 +43,36 @@ public AlterZoneEntry(CatalogZoneDescriptor descriptor) {
this.descriptor = descriptor;
}
- /** Returns descriptor of a zone to alter. */
+ /**
+ * Returns descriptor of a zone to alter.
+ */
public CatalogZoneDescriptor descriptor() {
return descriptor;
}
- /** {@inheritDoc} */
+ @Override
+ public CatalogEvent eventType() {
+ return CatalogEvent.ZONE_ALTER;
+ }
+
+ @Override
+ public CatalogEventParameters createEventParameters(long causalityToken, int catalogVersion) {
+ return new AlterZoneEventParameters(causalityToken, catalogVersion, descriptor);
+ }
+
+ @Override
+ public Catalog applyUpdate(Catalog catalog, VersionedUpdate update) {
+ return new Catalog(
+ update.version(),
+ update.activationTimestamp(),
+ catalog.objectIdGenState(),
+ catalog.zones().stream()
+ .map(z -> z.id() == descriptor.id() ? descriptor : z)
Review Comment:
Yep
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237562111
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AlterColumnEventParameters.java:
##########
@@ -23,32 +23,36 @@
* Create table event parameters contains a column descriptor for the modified column.
*/
public class AlterColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final CatalogTableColumnDescriptor columnDescriptor;
+ private final CatalogTableColumnDescriptor descriptor;
/**
* Constructor.
*
* @param causalityToken Causality token.
- * @param tableId Returns an id the table to be modified.
- * @param columnDescriptor Descriptor for the column to be replaced.
+ * @param catalogVersion Catalog version.
+ * @param tableId Returns ID the table to be modified.
+ * @param descriptor Descriptor for the column to be replaced.
*/
- public AlterColumnEventParameters(long causalityToken, int tableId, CatalogTableColumnDescriptor columnDescriptor) {
- super(causalityToken);
+ public AlterColumnEventParameters(long causalityToken, int catalogVersion, int tableId, CatalogTableColumnDescriptor descriptor) {
+ super(causalityToken, catalogVersion);
this.tableId = tableId;
- this.columnDescriptor = columnDescriptor;
+ this.descriptor = descriptor;
}
- /** Returns an id of a modified table. */
+ /**
+ * Returns ID of a modified table.
+ */
Review Comment:
It is grammatically correct to use ID.
--
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
[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1237108035
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1248,19 +1275,22 @@ public void testCreateIndexEvents() {
// Create index.
assertThat(service.createIndex(createIndexParams), willCompleteSuccessfully());
- verify(eventListener).notify(any(CreateIndexEventParameters.class), ArgumentMatchers.isNull());
+ verify(eventListener, times(2)).notify(any(CreateIndexEventParameters.class), isNull());
// Drop index.
assertThat(service.dropIndex(dropIndexParams), willCompleteSuccessfully());
- verify(eventListener).notify(any(DropIndexEventParameters.class), ArgumentMatchers.isNull());
+ verify(eventListener).notify(any(DropIndexEventParameters.class), isNull());
+
+ clearInvocations(eventListener);
// Drop table.
assertThat(service.dropTable(dropTableparams), willCompleteSuccessfully());
// Try drop index once again.
assertThat(service.dropIndex(dropIndexParams), willThrow(IndexNotFoundException.class));
- verifyNoMoreInteractions(eventListener);
+ // Should have destroyed the pk index.
+ verify(eventListener).notify(any(DropIndexEventParameters.class), isNull());
Review Comment:
It is not clear, if `DropIndexEventParameters` was caused by `dropTable` or `dropIndex` call.
Let's move this higher.
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238018549
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/AddColumnEventParameters.java:
##########
@@ -24,25 +24,27 @@
* Add column event parameters contains descriptors of added columns.
*/
public class AddColumnEventParameters extends CatalogEventParameters {
-
private final int tableId;
- private final List<CatalogTableColumnDescriptor> columnDescriptors;
+ private final List<CatalogTableColumnDescriptor> descriptors;
/**
* Constructor.
*
* @param causalityToken Causality token.
- * @param tableId An id of table, which columns are added to.
- * @param columnDescriptors New columns descriptors.
+ * @param catalogVersion Catalog version.
+ * @param tableId ID of table, which columns are added to.
+ * @param descriptors New columns descriptors.
*/
- public AddColumnEventParameters(long causalityToken, int tableId, List<CatalogTableColumnDescriptor> columnDescriptors) {
- super(causalityToken);
+ public AddColumnEventParameters(long causalityToken, int catalogVersion, int tableId, List<CatalogTableColumnDescriptor> descriptors) {
+ super(causalityToken, catalogVersion);
this.tableId = tableId;
- this.columnDescriptors = columnDescriptors;
+ this.descriptors = descriptors;
}
- /** Returns table id. */
+ /**
+ * Returns table ID.
+ */
Review Comment:
single-line javadocs are perfectly fine, so, please, revert all such changes
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2231: IGNITE-19641 Catalog events are triggered too early.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2231:
URL: https://github.com/apache/ignite-3/pull/2231#discussion_r1238020934
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -150,66 +138,61 @@ public CatalogServiceImpl(UpdateLog updateLog, HybridClock clock, long delayDura
this.delayDurationMs = delayDurationMs;
}
- /** {@inheritDoc} */
Review Comment:
why did you remove this?
--
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