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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
