[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

["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