zstan commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086090495
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DistributionZoneSqlToCommandConverterTest.java:
##########
@@ -262,20 +248,82 @@ public void testCreateZoneWithReplicasAll(boolean
withPresent) throws SqlParseEx
? "CREATE ZONE test WITH STORAGE_PROFILES='" +
DEFAULT_STORAGE_PROFILE + "', REPLICAS=ALL"
: "CREATE ZONE test (REPLICAS ALL) STORAGE PROFILES ['" +
DEFAULT_STORAGE_PROFILE + "']";
- SqlNode node = parse(sql);
+ CatalogCommand cmd = convert(sql);
- assertThat(node, instanceOf(SqlDdl.class));
+ CatalogZoneDescriptor desc = invokeAndGetFirstEntry(cmd,
NewZoneEntry.class).descriptor();
- CatalogCommand cmd = converter.convert((SqlDdl) node, createContext());
+ assertThat(desc.replicas(),
equalTo(DistributionAlgorithm.ALL_REPLICAS));
+ }
+
+ private static List<Arguments> defaultQuorum() {
+ return List.of(
+ Arguments.of(1, 1),
+ Arguments.of(2, 2),
+ Arguments.of(5, 3),
+ Arguments.of(10, 3)
+ );
+ }
+
+ @ParameterizedTest(name = "with replicas = {0}")
+ @MethodSource("defaultQuorum")
+ public void testCreateZoneWithQuorumDefault(int replicas, int
expectedQuorum) throws SqlParseException {
+ String sql = "CREATE ZONE test WITH REPLICAS=" + replicas + ",
STORAGE_PROFILES='" + DEFAULT_STORAGE_PROFILE + "'";
+
+ CatalogCommand cmd = convert(sql);
CatalogZoneDescriptor desc = invokeAndGetFirstEntry(cmd,
NewZoneEntry.class).descriptor();
- assertThat(desc.replicas(),
equalTo(DistributionAlgorithm.ALL_REPLICAS));
+ assertThat(desc.quorumSize(), equalTo(expectedQuorum));
+ }
+
+ private static List<Arguments> quorumSize() {
Review Comment:
```suggestion
private static List<Arguments> correctQuorumSize() {
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -192,6 +195,8 @@ public DdlSqlToCommandConverter() {
// ALTER ZONE options.
alterZoneOptionInfos = new EnumMap<>(Map.of(
PARTITIONS, new DdlOptionInfo<>(Integer.class,
this::checkPositiveNumber, AlterZoneCommandBuilder::partitions),
+ // We can't properly validate quorum size since it depends on
the replicas number.
+ QUORUM_SIZE, new DdlOptionInfo<>(Integer.class,
this::checkPositiveNumber, AlterZoneCommandBuilder::quorumSize),
Review Comment:
the same for check
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DistributionZoneSqlToCommandConverterTest.java:
##########
@@ -262,20 +248,82 @@ public void testCreateZoneWithReplicasAll(boolean
withPresent) throws SqlParseEx
? "CREATE ZONE test WITH STORAGE_PROFILES='" +
DEFAULT_STORAGE_PROFILE + "', REPLICAS=ALL"
: "CREATE ZONE test (REPLICAS ALL) STORAGE PROFILES ['" +
DEFAULT_STORAGE_PROFILE + "']";
- SqlNode node = parse(sql);
+ CatalogCommand cmd = convert(sql);
- assertThat(node, instanceOf(SqlDdl.class));
+ CatalogZoneDescriptor desc = invokeAndGetFirstEntry(cmd,
NewZoneEntry.class).descriptor();
- CatalogCommand cmd = converter.convert((SqlDdl) node, createContext());
+ assertThat(desc.replicas(),
equalTo(DistributionAlgorithm.ALL_REPLICAS));
+ }
+
+ private static List<Arguments> defaultQuorum() {
+ return List.of(
+ Arguments.of(1, 1),
+ Arguments.of(2, 2),
+ Arguments.of(5, 3),
+ Arguments.of(10, 3)
+ );
+ }
+
+ @ParameterizedTest(name = "with replicas = {0}")
Review Comment:
```suggestion
@ParameterizedTest(name = "replicas = {0}, expectedQuorum = {1}")
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/ZoneOptionEnum.java:
##########
@@ -28,6 +28,9 @@ public enum ZoneOptionEnum {
/** Number of replicas. */
REPLICAS("REPLICAS"),
+ /** Number of replicas. */
Review Comment:
not a replicas but ... ?)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DistributionZoneSqlToCommandConverterTest.java:
##########
@@ -184,21 +171,24 @@ public void testCreateZoneWithOptions(boolean
withPresent) throws SqlParseExcept
+ "storage_profiles='lru_rocks ,
segmented_aipersist ' "
: "CREATE ZONE test "
+ "(partitions 2, "
- + "replicas 3, "
+ + "replicas 5, "
+ + "quorum size 2, " // non-default value
+ "distribution algorithm 'rendezvous', "
+ "nodes filter '$[?(@.region == \"US\")]', "
+ "auto adjust 300) "
+ "storage profiles ['lru_rocks',
'segmented_aipersist '] ";
- SqlNode node = parse(sql);
-
- assertThat(node, instanceOf(SqlDdl.class));
-
- CatalogCommand cmd = converter.convert((SqlDdl) node,
createContext());
+ CatalogCommand cmd = convert(sql);
CatalogZoneDescriptor desc = invokeAndGetFirstEntry(cmd,
NewZoneEntry.class).descriptor();
assertThat(desc.partitions(), equalTo(2));
- assertThat(desc.replicas(), equalTo(3));
+ if (withPresent) {
+ // There's no quorum size with "with" syntax, so keep default
Review Comment:
1. It confusing me, "with" syntax is not deprecated just a bit outdated
form, thus if someone already use it - what he need to do in such a case, do we
have related documentation issue ?
2. It still possible to set quorum_size=2 with "with" syntax
3. Probably it not related to current issue but error message : "Specified
quorum size doesn't fit into the specified replicas count: [quorum size=1,
min=2, max=2, replicas count=3]" confusing, what did i need to do here from
maintenance side ? If you agree - we need additional issue here.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -174,6 +175,8 @@ public DdlSqlToCommandConverter() {
// CREATE ZONE options.
zoneOptionInfos = new EnumMap<>(Map.of(
PARTITIONS, new DdlOptionInfo<>(Integer.class,
this::checkPositiveNumber, CreateZoneCommandBuilder::partitions),
+ // We can't properly validate quorum size since it depends on
the replicas number.
+ QUORUM_SIZE, new DdlOptionInfo<>(Integer.class,
this::checkPositiveNumber, CreateZoneCommandBuilder::quorumSize),
Review Comment:
you already has such a check (checkPositiveNumber) on parser level
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DistributionZoneSqlToCommandConverterTest.java:
##########
@@ -184,21 +171,24 @@ public void testCreateZoneWithOptions(boolean
withPresent) throws SqlParseExcept
+ "storage_profiles='lru_rocks ,
segmented_aipersist ' "
: "CREATE ZONE test "
+ "(partitions 2, "
- + "replicas 3, "
+ + "replicas 5, "
+ + "quorum size 2, " // non-default value
+ "distribution algorithm 'rendezvous', "
+ "nodes filter '$[?(@.region == \"US\")]', "
+ "auto adjust 300) "
+ "storage profiles ['lru_rocks',
'segmented_aipersist '] ";
- SqlNode node = parse(sql);
-
- assertThat(node, instanceOf(SqlDdl.class));
-
- CatalogCommand cmd = converter.convert((SqlDdl) node,
createContext());
+ CatalogCommand cmd = convert(sql);
CatalogZoneDescriptor desc = invokeAndGetFirstEntry(cmd,
NewZoneEntry.class).descriptor();
assertThat(desc.partitions(), equalTo(2));
- assertThat(desc.replicas(), equalTo(3));
+ if (withPresent) {
+ // There's no quorum size with "with" syntax, so keep default
Review Comment:
One more ... seems someone can set :
replicas=ALL and ...
CreateZoneCommand#validateReplicasAndQuorumCompatibility will allow huge range
for quorum but it`s not correct, isn`t 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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
