sanpwc commented on code in PR #1799:
URL: https://github.com/apache/ignite-3/pull/1799#discussion_r1145311424
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/configuration/DistributionZoneConfigurationSchema.java:
##########
@@ -41,6 +43,16 @@ public class DistributionZoneConfigurationSchema {
@Value(hasDefault = true)
public int zoneId = DEFAULT_ZONE_ID;
+ /** Zone partitions. */
+ @Range(min = 0, max = 65_000)
+ @Value(hasDefault = true)
+ public int partitions = DEFAULT_PARTITION_COUNT;
Review Comment:
Please create corresponding GG ticket that will add validation on matching
partitions for primary and secondary storages.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/configuration/DistributionZoneConfigurationSchema.java:
##########
@@ -41,6 +43,16 @@ public class DistributionZoneConfigurationSchema {
@Value(hasDefault = true)
public int zoneId = DEFAULT_ZONE_ID;
+ /** Zone partitions. */
Review Comment:
Minor. I'd rather be consistent. It's either "Count of zone partitions."
like in "Count of zone partition replicas." or "Count of zone partition
replicas." is just "Zone partition replicas."
##########
modules/rest-api/openapi/openapi.yaml:
##########
@@ -327,7 +327,7 @@ paths:
parameters: []
responses:
"200":
- description: All statutes returned successful.
+ description: All statutes returned successfully.
Review Comment:
Is it an intended change, or an artifact?
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/index/PersistentPageMemorySortedIndexStorageTest.java:
##########
@@ -59,7 +62,8 @@ void setUp(
engine.start();
- table = engine.createMvTable(tablesConfig.tables().get("foo"),
tablesConfig);
+ table = engine.createMvTable(tablesConfig.tables().get("foo"),
tablesConfig,
Review Comment:
Here and in other places. Instead of adding partitions parameter to
createMVTable I'd actually add zonesCfg and change createMvTable internals in
order to retrieve partition values and similar from within zone cfg, where zone
itself will be matched by table.zoneName binding. Actually speaking, seems that
after moving storageCfg to zoneCfg we will update createMvTable one more timer,
probably removing tablesCondig from the parameters list, only zoneCfg will be
used. Not sure though.
##########
modules/storage-rocksdb/build.gradle:
##########
@@ -35,6 +35,7 @@ dependencies {
testAnnotationProcessor
project(':ignite-configuration-annotation-processor')
testImplementation project(':ignite-core')
+ testImplementation project(':ignite-distribution-zones')
Review Comment:
Why?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java:
##########
@@ -202,6 +206,11 @@ public TablesConfiguration tablesConfiguration() {
return tablesCfg;
}
+ @Override
+ public int partitions() {
Review Comment:
And according to aforementioned comment that assumes that it's better to
have ZoneCfg instead of partitions it's also better to add
`ZoneConfiguration configuration();` that will probably substitute
`TableConfiguration configuration(); ` when we will migrate storage cfg into
zoneCfg.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/configuration/DistributionZoneConfigurationSchema.java:
##########
@@ -41,6 +43,16 @@ public class DistributionZoneConfigurationSchema {
@Value(hasDefault = true)
public int zoneId = DEFAULT_ZONE_ID;
+ /** Zone partitions. */
+ @Range(min = 0, max = 65_000)
Review Comment:
min = 0, really? why?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java:
##########
@@ -99,6 +98,9 @@ public class RocksDbTableStorage implements MvTableStorage {
/** Data region for the table. */
private final RocksDbDataRegion dataRegion;
+ /** Number of storage partitions. */
+ private final int partitions;
Review Comment:
Same as above. Instead of partitions, I'd rather add ZoneConfiguration
because it'll probably substitute TableConfiguration within storages.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
