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/23 13:34:39 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request, #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
tkalkirill opened a new pull request, #2247:
URL: https://github.com/apache/ignite-3/pull/2247
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241843388
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -275,11 +281,16 @@ public CompletableFuture<Void> createTable(CreateTableParams params) {
CatalogZoneDescriptor zone = getZone(catalog, Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
- CatalogTableDescriptor table = CatalogUtils.fromParams(catalog.objectIdGenState(), zone.id(), params);
+ int id = catalog.objectIdGenState();
+
+ CatalogTableDescriptor table = CatalogUtils.fromParams(id++, zone.id(), params);
+
+ CatalogHashIndexDescriptor pkIndex = createHashIndexDescriptor(table, id++, createPkIndexParams(params));
Review Comment:
I don't understand why it's not consistent.
I don't see any problem with other methods.
--
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241857900
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -44,6 +44,8 @@ public interface CatalogService {
CatalogTableDescriptor table(int tableId, long timestamp);
+ CatalogTableDescriptor table(int tableId, int catalogVersion);
Review Comment:
> We should discuss it and probably implement it later.
That's what I proposed, to do it later.
I don't think that the fact that it's a feature branch changes anything, to be honest, but that's out of the scope of the discussion
--
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241861239
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -295,7 +306,7 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
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:
It's not redundant, it's how it's supposed to work. Passing data that's already available elsewhere upon processing is redundant
--
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241839866
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -877,4 +886,24 @@ private static void validateAlterTableColumn(
throwUnsupportedDdl("Cannot decrease precision to {} for column '{}'.", target.precision(), origin.name());
}
}
+
+ private static CreateHashIndexParams createPkIndexParams(CreateTableParams params) {
+ return CreateHashIndexParams.builder()
+ .schemaName(params.schemaName())
+ .tableName(params.tableName())
+ .indexName(params.tableName() + "_PK")
+ .columns(params.primaryKeyColumns())
+ .unique()
+ .build();
+ }
+
+ private static CatalogHashIndexDescriptor createHashIndexDescriptor(
Review Comment:
I don't see a problem here.
We have two places where this code is used, this is already a reason to use 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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241830620
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1638,6 +1681,78 @@ void testGetCatalogEntityInCatalogEvent() {
assertThat(result, willCompleteSuccessfully());
}
+ @Test
+ void testGetTableByIdAndCatalogVersion() {
+ assertThat(service.createTable(simpleTable(TABLE_NAME)), willBe(nullValue()));
+
+ assertNull(service.table(2, 0));
+ assertNotNull(service.table(2, 1));
+ }
+
+ @Test
+ void testGetTableIdOnDropIndexEvent() {
+ assertThat(service.createTable(simpleTable(TABLE_NAME)), willBe(nullValue()));
+
+ assertThat(
+ service.createIndex(
+ CreateHashIndexParams.builder()
+ .schemaName(SCHEMA_NAME)
+ .tableName(TABLE_NAME)
+ .indexName(INDEX_NAME)
+ .columns(List.of("VAL"))
+ .build()
+ ),
+ willBe(nullValue())
+ );
+
+ EventListener<CatalogEventParameters> eventListener = mock(EventListener.class);
+
+ ArgumentCaptor<DropIndexEventParameters> captor = ArgumentCaptor.forClass(DropIndexEventParameters.class);
+
+ doReturn(completedFuture(false)).when(eventListener).notify(captor.capture(), any());
+
+ service.listen(CatalogEvent.INDEX_DROP, eventListener);
+
+ // Let's remove the index.
+ assertThat(
+ service.dropIndex(DropIndexParams.builder().schemaName(SCHEMA_NAME).indexName(INDEX_NAME).build()),
+ willBe(nullValue())
+ );
+
+ DropIndexEventParameters eventParameters = captor.getValue();
+
+ assertEquals(4L, eventParameters.indexId());
+ assertEquals(2L, eventParameters.tableId());
+
+ // Let's delete the table.
+ assertThat(
+ service.dropTable(DropTableParams.builder().schemaName(SCHEMA_NAME).tableName(TABLE_NAME).build()),
+ willBe(nullValue())
+ );
+
+ // Let's make sure that the pk index has been deleted.
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241830203
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1250,24 +1286,31 @@ public void testCreateIndexEvents() {
assertThat(service.createIndex(createIndexParams), willThrow(TableNotFoundException.class));
verifyNoInteractions(eventListener);
- // Create table.
+ // Create table with pk index.
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 merged pull request #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247
--
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241859836
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -275,11 +281,16 @@ public CompletableFuture<Void> createTable(CreateTableParams params) {
CatalogZoneDescriptor zone = getZone(catalog, Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
- CatalogTableDescriptor table = CatalogUtils.fromParams(catalog.objectIdGenState(), zone.id(), params);
+ int id = catalog.objectIdGenState();
+
+ CatalogTableDescriptor table = CatalogUtils.fromParams(id++, zone.id(), params);
+
+ CatalogHashIndexDescriptor pkIndex = createHashIndexDescriptor(table, id++, createPkIndexParams(params));
Review Comment:
Every other method uses pair
"validate" (optional)
"CatalogUtils.fromParams"
And hash indexes are the only ones that use "createHashIndexDescriptor". This is what's inconsistent here - different code patterns, lack of unified approach
--
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241861656
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -877,4 +886,24 @@ private static void validateAlterTableColumn(
throwUnsupportedDdl("Cannot decrease precision to {} for column '{}'.", target.precision(), origin.name());
}
}
+
+ private static CreateHashIndexParams createPkIndexParams(CreateTableParams params) {
+ return CreateHashIndexParams.builder()
+ .schemaName(params.schemaName())
+ .tableName(params.tableName())
+ .indexName(params.tableName() + "_PK")
+ .columns(params.primaryKeyColumns())
+ .unique()
+ .build();
+ }
+
+ private static CatalogHashIndexDescriptor createHashIndexDescriptor(
Review Comment:
Is it though?
--
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241845381
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -44,6 +44,8 @@ public interface CatalogService {
CatalogTableDescriptor table(int tableId, long timestamp);
+ CatalogTableDescriptor table(int tableId, int catalogVersion);
Review Comment:
For now, I need this method without complications.
I think we can think about your proposal later.
I remind you that this is a ticket for a feature branch.
--
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241826467
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -191,6 +191,12 @@ public CatalogTableDescriptor table(int tableId, long timestamp) {
return catalogAt(timestamp).table(tableId);
}
+ @Override
+ public CatalogTableDescriptor table(int tableId, int catalogVersion) {
+ return catalog(catalogVersion).table(tableId);
+ }
+
+ /** {@inheritDoc} */
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241836962
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -877,4 +886,24 @@ private static void validateAlterTableColumn(
throwUnsupportedDdl("Cannot decrease precision to {} for column '{}'.", target.precision(), origin.name());
}
}
+
+ private static CreateHashIndexParams createPkIndexParams(CreateTableParams params) {
+ return CreateHashIndexParams.builder()
Review Comment:
About the state of indexes will be continued in epic https://issues.apache.org/jira/browse/IGNITE-17766.
--
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241761480
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -44,6 +44,8 @@ public interface CatalogService {
CatalogTableDescriptor table(int tableId, long timestamp);
+ CatalogTableDescriptor table(int tableId, int catalogVersion);
Review Comment:
Are we sure that we need methods with version parameter? This will eventually multiply the number of methods by two. I would propose having a single "versionToTimestamp" method instead. We should discuss it and probably implement it later.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -275,11 +281,16 @@ public CompletableFuture<Void> createTable(CreateTableParams params) {
CatalogZoneDescriptor zone = getZone(catalog, Objects.requireNonNullElse(params.zone(), DEFAULT_ZONE_NAME));
- CatalogTableDescriptor table = CatalogUtils.fromParams(catalog.objectIdGenState(), zone.id(), params);
+ int id = catalog.objectIdGenState();
+
+ CatalogTableDescriptor table = CatalogUtils.fromParams(id++, zone.id(), params);
+
+ CatalogHashIndexDescriptor pkIndex = createHashIndexDescriptor(table, id++, createPkIndexParams(params));
Review Comment:
Table descriptor is created using "CatalogUtils.fromParams", and index is created using "createHash...", it's a little inconsistent. Can you please check other methods and what they do? It would be nice if all methods were similar to each other
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -191,6 +191,12 @@ public CatalogTableDescriptor table(int tableId, long timestamp) {
return catalogAt(timestamp).table(tableId);
}
+ @Override
+ public CatalogTableDescriptor table(int tableId, int catalogVersion) {
+ return catalog(catalogVersion).table(tableId);
+ }
+
+ /** {@inheritDoc} */
Review Comment:
You added `/** {@inheritDoc} */`? I can't believe my eyes!
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -877,4 +886,24 @@ private static void validateAlterTableColumn(
throwUnsupportedDdl("Cannot decrease precision to {} for column '{}'.", target.precision(), origin.name());
}
}
+
+ private static CreateHashIndexParams createPkIndexParams(CreateTableParams params) {
+ return CreateHashIndexParams.builder()
+ .schemaName(params.schemaName())
+ .tableName(params.tableName())
+ .indexName(params.tableName() + "_PK")
+ .columns(params.primaryKeyColumns())
+ .unique()
+ .build();
+ }
+
+ private static CatalogHashIndexDescriptor createHashIndexDescriptor(
Review Comment:
Are you sure that extracting this method helps the code readability? I don't think so:
- it's only two lines
- it's only used twice
- it breaks code flow by introducing different name and usage pattern (see one of the comments above)
What if we don't introduce it?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/DropZoneEventParameters.java:
##########
@@ -30,8 +30,8 @@ public class DropZoneEventParameters extends CatalogEventParameters {
* @param causalityToken Causality token.
* @param zoneId An id of dropped distribution zone.
*/
- public DropZoneEventParameters(long causalityToken, int zoneId) {
- super(causalityToken);
+ public DropZoneEventParameters(long causalityToken, int catalogVersion, int zoneId) {
Review Comment:
Please add catalog version to javadoc
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -877,4 +886,24 @@ private static void validateAlterTableColumn(
throwUnsupportedDdl("Cannot decrease precision to {} for column '{}'.", target.precision(), origin.name());
}
}
+
+ private static CreateHashIndexParams createPkIndexParams(CreateTableParams params) {
+ return CreateHashIndexParams.builder()
Review Comment:
Maybe I'm missing something, but where's the index state?
PK index must be created as an RW index, but such enum/classification doesn't seem to exist. Are we planning to implement it later?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -44,6 +44,8 @@ public interface CatalogService {
CatalogTableDescriptor table(int tableId, long timestamp);
+ CatalogTableDescriptor table(int tableId, int catalogVersion);
Review Comment:
Or the opposite, migrate all methods to versions. I don't have a clear answer
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1638,6 +1681,78 @@ void testGetCatalogEntityInCatalogEvent() {
assertThat(result, willCompleteSuccessfully());
}
+ @Test
+ void testGetTableByIdAndCatalogVersion() {
+ assertThat(service.createTable(simpleTable(TABLE_NAME)), willBe(nullValue()));
+
+ assertNull(service.table(2, 0));
+ assertNotNull(service.table(2, 1));
+ }
+
+ @Test
+ void testGetTableIdOnDropIndexEvent() {
+ assertThat(service.createTable(simpleTable(TABLE_NAME)), willBe(nullValue()));
+
+ assertThat(
+ service.createIndex(
+ CreateHashIndexParams.builder()
+ .schemaName(SCHEMA_NAME)
+ .tableName(TABLE_NAME)
+ .indexName(INDEX_NAME)
+ .columns(List.of("VAL"))
+ .build()
+ ),
+ willBe(nullValue())
+ );
+
+ EventListener<CatalogEventParameters> eventListener = mock(EventListener.class);
+
+ ArgumentCaptor<DropIndexEventParameters> captor = ArgumentCaptor.forClass(DropIndexEventParameters.class);
+
+ doReturn(completedFuture(false)).when(eventListener).notify(captor.capture(), any());
+
+ service.listen(CatalogEvent.INDEX_DROP, eventListener);
+
+ // Let's remove the index.
+ assertThat(
+ service.dropIndex(DropIndexParams.builder().schemaName(SCHEMA_NAME).indexName(INDEX_NAME).build()),
+ willBe(nullValue())
+ );
+
+ DropIndexEventParameters eventParameters = captor.getValue();
+
+ assertEquals(4L, eventParameters.indexId());
+ assertEquals(2L, eventParameters.tableId());
+
+ // Let's delete the table.
+ assertThat(
+ service.dropTable(DropTableParams.builder().schemaName(SCHEMA_NAME).tableName(TABLE_NAME).build()),
+ willBe(nullValue())
+ );
+
+ // Let's make sure that the pk index has been deleted.
Review Comment:
```suggestion
// Let's make sure that the PK index has been deleted.
```
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1250,24 +1286,31 @@ public void testCreateIndexEvents() {
assertThat(service.createIndex(createIndexParams), willThrow(TableNotFoundException.class));
verifyNoInteractions(eventListener);
- // Create table.
+ // Create table with pk index.
Review Comment:
```suggestion
// Create table with PK index.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -295,7 +306,7 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
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:
Isn't this information excessive?
Does alive index have the "tableId", so that you could read it before deletion?
--
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241829741
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/DropZoneEventParameters.java:
##########
@@ -30,8 +30,8 @@ public class DropZoneEventParameters extends CatalogEventParameters {
* @param causalityToken Causality token.
* @param zoneId An id of dropped distribution zone.
*/
- public DropZoneEventParameters(long causalityToken, int zoneId) {
- super(causalityToken);
+ public DropZoneEventParameters(long causalityToken, int catalogVersion, int zoneId) {
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 #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241839866
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -877,4 +886,24 @@ private static void validateAlterTableColumn(
throwUnsupportedDdl("Cannot decrease precision to {} for column '{}'.", target.precision(), origin.name());
}
}
+
+ private static CreateHashIndexParams createPkIndexParams(CreateTableParams params) {
+ return CreateHashIndexParams.builder()
+ .schemaName(params.schemaName())
+ .tableName(params.tableName())
+ .indexName(params.tableName() + "_PK")
+ .columns(params.primaryKeyColumns())
+ .unique()
+ .build();
+ }
+
+ private static CatalogHashIndexDescriptor createHashIndexDescriptor(
Review Comment:
I don't see a problem 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] tkalkirill commented on a diff in pull request #2247: IGNITE-19798 Add functionality to the catalog to switch the IndexManager to catalog events
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2247:
URL: https://github.com/apache/ignite-3/pull/2247#discussion_r1241839211
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -295,7 +306,7 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
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:
Only the indexId being deleted gets into the event, and to get it you need to refer to the previous version of the catalog, which seems a little more redundant than just passing the tableId.
--
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