You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "valepakh (via GitHub)" <gi...@apache.org> on 2025/05/12 08:19:18 UTC
[PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
valepakh opened a new pull request, #5795:
URL: https://github.com/apache/ignite-3/pull/5795
https://issues.apache.org/jira/browse/IGNITE-24391
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086322057
##########
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:
> It confusing me, "with" syntax is not deprecated just a bit outdated form
In the [IGNITE-24708](https://issues.apache.org/jira/browse/IGNITE-24708) the proposal was
> Keep current sytax as is, and freeze it for any changes in future.
That's why I thought that we shouldn't add any more options to it. Looks like I understood it differently?
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086563745
##########
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:
> It still possible to set quorum_size=2 with "with" syntax
Right, added some more tests cases for that
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086371801
##########
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:
FIxed
##########
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:
Fixed
##########
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:
Fixed
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086693763
##########
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:
Added system view. There's also a catalog DSL, I've a follow-up ticket for that.
##########
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:
Added quorum size to system view. There's also a catalog DSL, I've a follow-up ticket for that.
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086089661
##########
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086352043
##########
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:
> replicas=ALL and ... CreateZoneCommand#validateReplicasAndQuorumCompatibility will allow huge range for quorum but it's not correct, isn't it ?
Could you please elaborate, why it's not correct?
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086372248
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/DistributionZoneSqlDdlParserTest.java:
##########
@@ -92,6 +92,7 @@ public void createZoneWithOptions() {
IgniteSqlCreateZone createZone = parseCreateZone(
"create zone test_zone "
+ "(replicas 2, "
+ + "quorum size 2, "
Review Comment:
Added
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086135345
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/DistributionZoneSqlDdlParserTest.java:
##########
@@ -92,6 +92,7 @@ public void createZoneWithOptions() {
IgniteSqlCreateZone createZone = parseCreateZone(
"create zone test_zone "
+ "(replicas 2, "
+ + "quorum size 2, "
Review Comment:
let's also include new property into `org.apache.ignite.internal.sql.engine.sql.DistributionZoneSqlDdlParserTest#alterZoneSetOptions`
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086322510
##########
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:
But we have the same checks for other positive integers, like partitions or auto adjust. Should I remove it or should I keep it consistent with the current code?
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086694850
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateZoneCommandValidationTest.java:
##########
@@ -152,6 +152,29 @@ void zoneQuorumSize(
}
}
+ @Test
+ void extremeQuorumSize() {
+ int replicas = Integer.MAX_VALUE; // REPLICAS = ALL
+ int maxQuorumSize = Integer.MAX_VALUE / 2 + 1;
+ int defaultQuorumSize = 3;
Review Comment:
Used actual calculations there
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086654803
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateZoneCommandValidationTest.java:
##########
@@ -152,6 +152,29 @@ void zoneQuorumSize(
}
}
+ @Test
+ void extremeQuorumSize() {
+ int replicas = Integer.MAX_VALUE; // REPLICAS = ALL
+ int maxQuorumSize = Integer.MAX_VALUE / 2 + 1;
Review Comment:
This is not equivalent. It works only for odd number of replicas.
--
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
Re: [PR] IGNITE-24391 Introduce the new QUORUM_SIZE parameter into CREATE ZONE and ALTER ZONE commands [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #5795:
URL: https://github.com/apache/ignite-3/pull/5795#discussion_r2086512444
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateZoneCommandValidationTest.java:
##########
@@ -152,6 +152,29 @@ void zoneQuorumSize(
}
}
+ @Test
+ void extremeQuorumSize() {
+ int replicas = Integer.MAX_VALUE; // REPLICAS = ALL
+ int maxQuorumSize = Integer.MAX_VALUE / 2 + 1;
+ int defaultQuorumSize = 3;
Review Comment:
```suggestion
int defaultQuorumSize = defaultQuorumSize(replicas);
```
##########
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:
> Looks like I understood it differently?
discussed privately
##########
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:
you need to change ItZonesSystemViewTest and all correspondings
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateZoneCommandValidationTest.java:
##########
@@ -152,6 +152,29 @@ void zoneQuorumSize(
}
}
+ @Test
+ void extremeQuorumSize() {
+ int replicas = Integer.MAX_VALUE; // REPLICAS = ALL
+ int maxQuorumSize = Integer.MAX_VALUE / 2 + 1;
Review Comment:
can you change this : int maxQuorum = max(minQuorum, (int) (round(replicas / 2.0))); from
CreateZoneCommand#validateReplicasAndQuorumCompatibility into : "replicas / 2 + 1" ?
it become more clear, but it`s up to you here. I mean - change "round" for " / 2 + 1"
##########
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:
ok, seems we need to store it while "old" syntax still available
--
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