You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/12/09 00:17:09 UTC
[GitHub] [geode] kirklund opened a new pull request #7177: GEODE-9879: Extract SystemProperty to geode-common
kirklund opened a new pull request #7177:
URL: https://github.com/apache/geode/pull/7177
PROBLEM
SystemPropertyHelper combines dual-prefix property lookup with
product system property definitions. Property lookup is currently
limited to geode-core and not useable from modules that are upstream
from geode-core.
SOLUTION
Extract part of SystemPropertyHelper to geode-common for use in
modules that are upstream from geode-core.
Define new DEFAULT_PREFIX that maps to GEODE_PREFIX and use that
everywhere that GEODE_PREFIX was previously used.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund merged pull request #7177: GEODE-9879: Extract SystemProperty to geode-common
Posted by GitBox <gi...@apache.org>.
kirklund merged pull request #7177:
URL: https://github.com/apache/geode/pull/7177
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #7177: GEODE-9879: Extract SystemProperty to geode-common
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7177:
URL: https://github.com/apache/geode/pull/7177#discussion_r766023780
##########
File path: geode-core/src/main/java/org/apache/geode/internal/lang/SystemPropertyHelper.java
##########
@@ -25,8 +22,10 @@
*/
public class SystemPropertyHelper {
- public static final String GEODE_PREFIX = "geode.";
- public static final String GEMFIRE_PREFIX = "gemfire.";
+ @SuppressWarnings("unused")
+ public static final String GEODE_PREFIX = SystemProperty.GEODE_PREFIX;
Review comment:
I agree that we should just remove it. The path forward is to use DEFAULT_PREFIX instead of GEODE_PREFIX.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #7177: GEODE-9879: Extract SystemProperty to geode-common
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7177:
URL: https://github.com/apache/geode/pull/7177#discussion_r766055958
##########
File path: geode-core/src/main/java/org/apache/geode/internal/lang/SystemPropertyHelper.java
##########
@@ -25,8 +22,10 @@
*/
public class SystemPropertyHelper {
- public static final String GEODE_PREFIX = "geode.";
- public static final String GEMFIRE_PREFIX = "gemfire.";
+ @SuppressWarnings("unused")
+ public static final String GEODE_PREFIX = SystemProperty.GEODE_PREFIX;
Review comment:
Removed.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #7177: GEODE-9879: Extract SystemProperty to geode-common
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7177:
URL: https://github.com/apache/geode/pull/7177#discussion_r766020291
##########
File path: geode-common/build.gradle
##########
@@ -33,6 +33,9 @@ dependencies {
implementation('com.fasterxml.jackson.core:jackson-databind')
// test
+ testImplementation('com.github.stefanbirkner:system-rules') {
Review comment:
Yes. We already use system-rules in the other geode modules.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #7177: GEODE-9879: Extract SystemProperty to geode-common
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7177:
URL: https://github.com/apache/geode/pull/7177#discussion_r766057036
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/BrokenSerializationConsistencyRegressionTest.java
##########
@@ -15,8 +15,8 @@
package org.apache.geode.internal.cache;
import static org.apache.geode.cache.RegionShortcut.REPLICATE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_PREFIX;
Review comment:
I removed GEODE_PREFIX and inlined DEFAULT_PREFIX and GEMFIRE_PREFIX. Now all of the classes that referenced the prefixes via SystemPropertyHelper are referencing them from SystemProperty instead.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #7177: GEODE-9879: Extract SystemProperty to geode-common
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #7177:
URL: https://github.com/apache/geode/pull/7177#discussion_r766072373
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/BrokenSerializationConsistencyRegressionTest.java
##########
@@ -15,8 +15,8 @@
package org.apache.geode.internal.cache;
import static org.apache.geode.cache.RegionShortcut.REPLICATE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_PREFIX;
Review comment:
Yeah, eventually it's better to have a single class for the system properties. Having two classes is confusing which class should own which properties.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #7177: GEODE-9879: Extract SystemProperty to geode-common
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #7177:
URL: https://github.com/apache/geode/pull/7177#discussion_r765351757
##########
File path: geode-common/build.gradle
##########
@@ -33,6 +33,9 @@ dependencies {
implementation('com.fasterxml.jackson.core:jackson-databind')
// test
+ testImplementation('com.github.stefanbirkner:system-rules') {
Review comment:
Is the 3rd party library open source license compatible with Geode?
##########
File path: geode-core/src/main/java/org/apache/geode/internal/lang/SystemPropertyHelper.java
##########
@@ -25,8 +22,10 @@
*/
public class SystemPropertyHelper {
- public static final String GEODE_PREFIX = "geode.";
- public static final String GEMFIRE_PREFIX = "gemfire.";
+ @SuppressWarnings("unused")
+ public static final String GEODE_PREFIX = SystemProperty.GEODE_PREFIX;
Review comment:
Why not remove unused `GEODE_PREFIX`?
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/BrokenSerializationConsistencyRegressionTest.java
##########
@@ -15,8 +15,8 @@
package org.apache.geode.internal.cache;
import static org.apache.geode.cache.RegionShortcut.REPLICATE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_PREFIX;
Review comment:
Why not importing the new `SystemProperty.DEFAULT_PREFIX`? Since the new `SystemProperty` class is introduced. My understanding is that the new class `SystemProperty` will be used instead of the existing `SystemPropertyHelper`. And `SystemPropertyHelper.DEFAULT_PREFIX` is essentially `SystemProperty.DEFAULT_PREFIX`. I see there are quite a few places in this pull request that use `SystemPropertyHelper.DEFAULT_PREFIX`.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #7177: GEODE-9879: Extract SystemProperty to geode-common
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7177:
URL: https://github.com/apache/geode/pull/7177#discussion_r766022472
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/BrokenSerializationConsistencyRegressionTest.java
##########
@@ -15,8 +15,8 @@
package org.apache.geode.internal.cache;
import static org.apache.geode.cache.RegionShortcut.REPLICATE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_PREFIX;
Review comment:
I was trying to keep SystemPropertyHelper as a complete API such that any geode-core class that uses SystemPropertyHelper does not also need to import and use SystemProperty. We can however change the geode-core classes to use both classes.
I personally don't like SystemPropertyHelper. I think it should be deleted after moving all of the system properties into the classes that "own" those properties. Then those classes would use SystemProperty only for the dual prefixes of "geode." and "gemfire."
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org