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/13 08:11:06 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request, #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
tkalkirill opened a new pull request, #2184:
URL: https://github.com/apache/ignite-3/pull/2184
https://issues.apache.org/jira/browse/IGNITE-19483
--
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 #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228215004
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptor.java:
##########
@@ -75,6 +78,35 @@ public CatalogZoneDescriptor(
this.filter = filter;
}
+ /**
+ * Constructs a distribution zone descriptor.
+ *
+ * @param id Id of the distribution zone.
+ * @param name Name of the zone.
+ * @param partitions Amount of partitions in distributions zone.
+ * @param replicas Amount of partition replicas.
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 #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228217596
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##########
@@ -154,21 +140,11 @@ protected final void initialize(
}
@AfterEach
- void tearDown() throws Exception {
- IgniteUtils.closeAll(
- tableStorage == null ? null : tableStorage::stop,
- storageEngine == null ? null : storageEngine::stop
- );
- }
-
- protected MvTableStorage createMvTableStorage(TablesConfiguration tablesConfig,
- DistributionZoneConfiguration distributionZoneConfiguration) {
- return storageEngine.createMvTable(getTableConfig(tablesConfig), tablesConfig, distributionZoneConfiguration);
+ protected void tearDown() throws Exception {
+ IgniteUtils.closeAllManually(tableStorage);
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 #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228220452
##########
modules/catalog/build.gradle:
##########
@@ -29,6 +29,7 @@ dependencies {
implementation project(':ignite-metastorage-api')
implementation project(':ignite-vault')
implementation project(':ignite-schema')
+ implementation project(':ignite-distribution-zones')
Review Comment:
This is a temporary dependency so that we can convert the table/index/zone configuration in "one place".
When we leave the configuration, this dependence should disappear in theory.
--
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 #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228216385
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1408,55 +1419,38 @@ public void testDefaultZone() {
// Validate default zone wasn't changed.
assertSame(defaultZone, service.zone(CatalogService.DEFAULT_ZONE_NAME, clock.nowLong()));
-
- // Try to rename to a zone with default name.
Review Comment:
This code is similar to copy-paste, this scenario is checked above.
--
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] rpuch commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228074486
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -1031,10 +1038,10 @@ public void handle(VersionedUpdate update) {
: new CatalogTableDescriptor(
table.id(),
table.name(),
+ table.zoneId(),
table.columns().stream()
- .map(source -> source.name().equals(target.name())
- ? target
- : source).collect(Collectors.toList()),
+ .map(source -> source.name().equals(target.name()) ? target : source)
+ .collect(Collectors.toList()),
Review Comment:
How about a static import?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogDescriptorUtils.java:
##########
@@ -188,4 +211,21 @@ private static CatalogIndexColumnDescriptor toIndexColumnDescriptor(IndexColumnV
return new CatalogIndexColumnDescriptor(config.name(), collation);
}
+
+ // TODO: IGNITE-19719 Fix it
+ private static CatalogDataStorageDescriptor toDataStorageDescriptor(DataStorageView config) {
+ String dataRegion;
+
+ try {
+ Method dataRegionMethod = config.getClass().getMethod("dataRegion");
Review Comment:
Is this caused by concrete configuration classes (rocksdb/aipersist/aimem) inavailability? How will it work when we move from the configuration completely? How will such polymorphic descriptors be built?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptor.java:
##########
@@ -75,6 +78,35 @@ public CatalogZoneDescriptor(
this.filter = filter;
}
+ /**
+ * Constructs a distribution zone descriptor.
+ *
+ * @param id Id of the distribution zone.
+ * @param name Name of the zone.
+ * @param partitions Amount of partitions in distributions zone.
+ * @param replicas Amount of partition replicas.
Review Comment:
```suggestion
* @param partitions Number of partitions in distributions zone.
* @param replicas Number of partition replicas.
```
##########
modules/catalog/build.gradle:
##########
@@ -29,6 +29,7 @@ dependencies {
implementation project(':ignite-metastorage-api')
implementation project(':ignite-vault')
implementation project(':ignite-schema')
+ implementation project(':ignite-distribution-zones')
Review Comment:
Why does the catalog module need to depend on the distribution zones module? Is it a temporary solution (until we stop using configurations to store schemas)?
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##########
@@ -154,21 +140,11 @@ protected final void initialize(
}
@AfterEach
- void tearDown() throws Exception {
- IgniteUtils.closeAll(
- tableStorage == null ? null : tableStorage::stop,
- storageEngine == null ? null : storageEngine::stop
- );
- }
-
- protected MvTableStorage createMvTableStorage(TablesConfiguration tablesConfig,
- DistributionZoneConfiguration distributionZoneConfiguration) {
- return storageEngine.createMvTable(getTableConfig(tablesConfig), tablesConfig, distributionZoneConfiguration);
+ protected void tearDown() throws Exception {
+ IgniteUtils.closeAllManually(tableStorage);
Review Comment:
How about calling `close()` directly here? `closeAll()` makes sense when there are at least 2 things to close
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##########
@@ -1408,55 +1419,38 @@ public void testDefaultZone() {
// Validate default zone wasn't changed.
assertSame(defaultZone, service.zone(CatalogService.DEFAULT_ZONE_NAME, clock.nowLong()));
-
- // Try to rename to a zone with default name.
Review Comment:
Was this scenario moved elsewhere? Or we don't need it amymore?
--
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 #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228209827
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -1031,10 +1038,10 @@ public void handle(VersionedUpdate update) {
: new CatalogTableDescriptor(
table.id(),
table.name(),
+ table.zoneId(),
table.columns().stream()
- .map(source -> source.name().equals(target.name())
- ? target
- : source).collect(Collectors.toList()),
+ .map(source -> source.name().equals(target.name()) ? target : source)
+ .collect(Collectors.toList()),
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 #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228213949
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogDescriptorUtils.java:
##########
@@ -188,4 +211,21 @@ private static CatalogIndexColumnDescriptor toIndexColumnDescriptor(IndexColumnV
return new CatalogIndexColumnDescriptor(config.name(), collation);
}
+
+ // TODO: IGNITE-19719 Fix it
+ private static CatalogDataStorageDescriptor toDataStorageDescriptor(DataStorageView config) {
+ String dataRegion;
+
+ try {
+ Method dataRegionMethod = config.getClass().getMethod("dataRegion");
Review Comment:
Yes, this "great" solution came from the unavailability of these engines, and at the moment I would not really like to add them to the dependency, since it will turn out to be a cyclic dependency.
I created IGNITE-19719 to figure out how we can do well.
--
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 #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184
--
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 #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228254928
##########
modules/catalog/build.gradle:
##########
@@ -29,6 +29,7 @@ dependencies {
implementation project(':ignite-metastorage-api')
implementation project(':ignite-vault')
implementation project(':ignite-schema')
+ implementation project(':ignite-distribution-zones')
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] rpuch commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228241311
##########
modules/catalog/build.gradle:
##########
@@ -29,6 +29,7 @@ dependencies {
implementation project(':ignite-metastorage-api')
implementation project(':ignite-vault')
implementation project(':ignite-schema')
+ implementation project(':ignite-distribution-zones')
Review Comment:
I think it's worth adding a TODO to this file then, to make sure we don't forget to remove the dependency
--
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