You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "alievmirza (via GitHub)" <gi...@apache.org> on 2023/04/04 18:18:51 UTC
[GitHub] [ignite-3] alievmirza opened a new pull request, #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
alievmirza opened a new pull request, #1902:
URL: https://github.com/apache/ignite-3/pull/1902
https://issues.apache.org/jira/browse/IGNITE-18952
--
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
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158525623
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/configuration/DistributionZoneConfigurationSchema.java:
##########
@@ -67,4 +67,8 @@ public class DistributionZoneConfigurationSchema {
@Range(min = 0)
@Value(hasDefault = true)
public int dataNodesAutoAdjustScaleDown = INFINITE_TIMER_VALUE;
+
+ @ValidFilter
+ @Value(hasDefault = true)
+ public String filter = "$..*";
Review Comment:
This is a com.jayway.jsonpath.JsonPath expression for including all attributes of nodes.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/configuration/DistributionZoneConfigurationSchema.java:
##########
@@ -67,4 +67,8 @@ public class DistributionZoneConfigurationSchema {
@Range(min = 0)
@Value(hasDefault = true)
public int dataNodesAutoAdjustScaleDown = INFINITE_TIMER_VALUE;
+
+ @ValidFilter
+ @Value(hasDefault = true)
+ public String filter = "$..*";
Review Comment:
This is a `com.jayway.jsonpath.JsonPath` expression for including all attributes of nodes.
--
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
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158311118
##########
modules/distribution-zones/build.gradle:
##########
@@ -37,6 +37,11 @@ dependencies {
implementation libs.jetbrains.annotations
implementation libs.fastutil.core
implementation libs.auto.service.annotations
+ implementation(libs.jsonpath.core) {
+ //IDEA test runner doesn't apply Gradle dependency resolve strategy, this is just not implemented
Review Comment:
Not sure, that I understand how the first line connected with the second, could you explain?
--
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
[GitHub] [ignite-3] sanpwc merged pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc merged PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902
--
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
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158573726
##########
modules/distribution-zones/build.gradle:
##########
@@ -37,6 +37,11 @@ dependencies {
implementation libs.jetbrains.annotations
implementation libs.fastutil.core
implementation libs.auto.service.annotations
+ implementation(libs.jsonpath.core) {
+ //IDEA test runner doesn't apply Gradle dependency resolve strategy, this is just not implemented
Review Comment:
I see, thanks!
--
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
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158520678
##########
modules/distribution-zones/build.gradle:
##########
@@ -37,6 +37,11 @@ dependencies {
implementation libs.jetbrains.annotations
implementation libs.fastutil.core
implementation libs.auto.service.annotations
+ implementation(libs.jsonpath.core) {
+ //IDEA test runner doesn't apply Gradle dependency resolve strategy, this is just not implemented
Review Comment:
This is a copy-paste from the other module where JsonPath was introduced, I think it's important to exclude this libs here as well
--
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
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158407429
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/configuration/DistributionZoneConfigurationSchema.java:
##########
@@ -67,4 +67,8 @@ public class DistributionZoneConfigurationSchema {
@Range(min = 0)
@Value(hasDefault = true)
public int dataNodesAutoAdjustScaleDown = INFINITE_TIMER_VALUE;
+
+ @ValidFilter
+ @Value(hasDefault = true)
+ public String filter = "$..*";
Review Comment:
What "$..*" means? Any String? Let's add a comment with an explanation.
--
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
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158413878
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerTest.java:
##########
@@ -755,17 +740,118 @@ public void testTryDropDefaultZone() {
);
}
- private void bindZoneToTable(String zoneName) {
- int zoneId = distributionZoneManager.getZoneId(zoneName);
+ @Test
+ public void testCreateZoneWithFilter() throws Exception {
+ String expectedFilter = "['nodeAttributes'][?(@.['region'] == 'EU')]";
+
+ distributionZoneManager.createZone(
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(expectedFilter).build()
+ )
+ .get(5, TimeUnit.SECONDS);
+
+ DistributionZoneConfiguration zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertEquals(expectedFilter, zone1.filter().value());
+
+ distributionZoneManager.dropZone(ZONE_NAME).get(5, TimeUnit.SECONDS);
+
+ zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertNull(zone1, "Zone was not dropped.");
+ }
+
+ @Test
+ public void testAlterZoneWithFilter() throws Exception {
+ String expectedFilter = "['nodeAttributes'][?(@.['region'] == 'EU')]";
+
+ distributionZoneManager.createZone(
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(expectedFilter).build()
+ )
+ .get(5, TimeUnit.SECONDS);
- NamedConfigurationTree<TableConfiguration, TableView, TableChange> tables = mock(NamedConfigurationTree.class, RETURNS_DEEP_STUBS);
+ DistributionZoneConfiguration zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertEquals(expectedFilter, zone1.filter().value());
+
+ String newExpectedFilter = "['nodeAttributes'][?(@.['storage'] == 'SSD')]";
+
+ distributionZoneManager.alterZone(
+ ZONE_NAME,
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(newExpectedFilter).build())
+ .get(5, TimeUnit.SECONDS);
+
+ zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertEquals(newExpectedFilter, zone1.filter().value());
+
+ distributionZoneManager.dropZone(ZONE_NAME).get(5, TimeUnit.SECONDS);
+
+ zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertNull(zone1, "Zone was not dropped.");
+ }
+
+ @Test
+ @SuppressWarnings("ThrowableNotThrown")
+ public void testCreateZoneWithNotValidFilter() {
+ assertThrowsWithCause(
+ () -> distributionZoneManager.createZone(
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter("['nodeAttributes'[?(@.['region'] == 'EU')]").build()
+ )
+ .get(5, TimeUnit.SECONDS),
+ ConfigurationValidationException.class
+ );
+ }
- when(tablesConfiguration.tables()).thenReturn(tables);
+ @Test
+ @SuppressWarnings("ThrowableNotThrown")
+ public void testAlterZoneWithNotValidFilter() throws Exception {
+ String expectedFilter = "['nodeAttributes'][?(@.['region'] == 'EU')]";
+
+ distributionZoneManager.createZone(
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(expectedFilter).build()
+ )
+ .get(5, TimeUnit.SECONDS);
- TableView tableView = mock(TableView.class);
+ DistributionZoneConfiguration zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertEquals(expectedFilter, zone1.filter().value());
+
+ String notValidFilter = "['nodeAttributes[?(@.['region'] == 'EU')]";
+
+ assertThrowsWithCause(
+ () -> distributionZoneManager.alterZone(
+ ZONE_NAME,
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(notValidFilter).build()
+ )
+ .get(5, TimeUnit.SECONDS),
+ ConfigurationValidationException.class
Review Comment:
Let's also check the message.
--
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
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158527317
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/configuration/DistributionZoneConfigurationSchema.java:
##########
@@ -67,4 +67,8 @@ public class DistributionZoneConfigurationSchema {
@Range(min = 0)
@Value(hasDefault = true)
public int dataNodesAutoAdjustScaleDown = INFINITE_TIMER_VALUE;
+
+ @ValidFilter
+ @Value(hasDefault = true)
+ public String filter = "$..*";
Review Comment:
yes, user have to pass `$..*` himself, if user wants to set default value
--
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
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158418721
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZonesUtil.java:
##########
@@ -347,4 +349,20 @@ public static DistributionZoneConfiguration getZoneById(DistributionZonesConfigu
throw new DistributionZoneNotFoundException(zoneId);
}
+
+ /**
+ * Check if a passed filter is a valid {@link JsonPath} query.
+ *
+ * @param filter Filter.
+ * @return {@code true} if the passed filter is a valid filter, {@code false} otherwise.
+ */
+ public static boolean validate(String filter) {
+ try {
+ JsonPath.compile(filter);
+ } catch (InvalidPathException ignored) {
Review Comment:
What about validation details? It's useful to propagate validation details to the user.
--
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
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158423185
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/configuration/DistributionZoneConfigurationSchema.java:
##########
@@ -67,4 +67,8 @@ public class DistributionZoneConfigurationSchema {
@Range(min = 0)
@Value(hasDefault = true)
public int dataNodesAutoAdjustScaleDown = INFINITE_TIMER_VALUE;
+
+ @ValidFilter
+ @Value(hasDefault = true)
+ public String filter = "$..*";
Review Comment:
And what if user will try to change the filter from some to "anything"? Do you expect them to specify "$..*"?
--
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
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #1902: IGNITE-18952 Add the filter as a configuration parameter of a Distribution zone
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #1902:
URL: https://github.com/apache/ignite-3/pull/1902#discussion_r1158525929
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerTest.java:
##########
@@ -755,17 +740,118 @@ public void testTryDropDefaultZone() {
);
}
- private void bindZoneToTable(String zoneName) {
- int zoneId = distributionZoneManager.getZoneId(zoneName);
+ @Test
+ public void testCreateZoneWithFilter() throws Exception {
+ String expectedFilter = "['nodeAttributes'][?(@.['region'] == 'EU')]";
+
+ distributionZoneManager.createZone(
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(expectedFilter).build()
+ )
+ .get(5, TimeUnit.SECONDS);
+
+ DistributionZoneConfiguration zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertEquals(expectedFilter, zone1.filter().value());
+
+ distributionZoneManager.dropZone(ZONE_NAME).get(5, TimeUnit.SECONDS);
+
+ zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertNull(zone1, "Zone was not dropped.");
+ }
+
+ @Test
+ public void testAlterZoneWithFilter() throws Exception {
+ String expectedFilter = "['nodeAttributes'][?(@.['region'] == 'EU')]";
+
+ distributionZoneManager.createZone(
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(expectedFilter).build()
+ )
+ .get(5, TimeUnit.SECONDS);
- NamedConfigurationTree<TableConfiguration, TableView, TableChange> tables = mock(NamedConfigurationTree.class, RETURNS_DEEP_STUBS);
+ DistributionZoneConfiguration zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertEquals(expectedFilter, zone1.filter().value());
+
+ String newExpectedFilter = "['nodeAttributes'][?(@.['storage'] == 'SSD')]";
+
+ distributionZoneManager.alterZone(
+ ZONE_NAME,
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(newExpectedFilter).build())
+ .get(5, TimeUnit.SECONDS);
+
+ zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertEquals(newExpectedFilter, zone1.filter().value());
+
+ distributionZoneManager.dropZone(ZONE_NAME).get(5, TimeUnit.SECONDS);
+
+ zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertNull(zone1, "Zone was not dropped.");
+ }
+
+ @Test
+ @SuppressWarnings("ThrowableNotThrown")
+ public void testCreateZoneWithNotValidFilter() {
+ assertThrowsWithCause(
+ () -> distributionZoneManager.createZone(
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter("['nodeAttributes'[?(@.['region'] == 'EU')]").build()
+ )
+ .get(5, TimeUnit.SECONDS),
+ ConfigurationValidationException.class
+ );
+ }
- when(tablesConfiguration.tables()).thenReturn(tables);
+ @Test
+ @SuppressWarnings("ThrowableNotThrown")
+ public void testAlterZoneWithNotValidFilter() throws Exception {
+ String expectedFilter = "['nodeAttributes'][?(@.['region'] == 'EU')]";
+
+ distributionZoneManager.createZone(
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(expectedFilter).build()
+ )
+ .get(5, TimeUnit.SECONDS);
- TableView tableView = mock(TableView.class);
+ DistributionZoneConfiguration zone1 = registry.getConfiguration(DistributionZonesConfiguration.KEY).distributionZones()
+ .get(ZONE_NAME);
+
+ assertEquals(expectedFilter, zone1.filter().value());
+
+ String notValidFilter = "['nodeAttributes[?(@.['region'] == 'EU')]";
+
+ assertThrowsWithCause(
+ () -> distributionZoneManager.alterZone(
+ ZONE_NAME,
+ new DistributionZoneConfigurationParameters.Builder(ZONE_NAME)
+ .filter(notValidFilter).build()
+ )
+ .get(5, TimeUnit.SECONDS),
+ ConfigurationValidationException.class
Review Comment:
done
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZonesUtil.java:
##########
@@ -347,4 +349,20 @@ public static DistributionZoneConfiguration getZoneById(DistributionZonesConfigu
throw new DistributionZoneNotFoundException(zoneId);
}
+
+ /**
+ * Check if a passed filter is a valid {@link JsonPath} query.
+ *
+ * @param filter Filter.
+ * @return {@code true} if the passed filter is a valid filter, {@code false} otherwise.
+ */
+ public static boolean validate(String filter) {
+ try {
+ JsonPath.compile(filter);
+ } catch (InvalidPathException ignored) {
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