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 2020/05/04 22:24:03 UTC
[GitHub] [geode] DonalEvans opened a new pull request #5049: GEODE-7864: Replace uses of hardcoded "/" with Region.SEPARATOR
DonalEvans opened a new pull request #5049:
URL: https://github.com/apache/geode/pull/5049
For greater consistency across the codebase, the Region.SEPARATOR and Region.SEPARATOR_CHAR constants should be used instead of hardcoded "/" and '/' in region names/paths.
Authored-by: Donal Evans <do...@pivotal.io>
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jujoramos edited a comment on pull request #5049: GEODE-7864: Replace uses of hardcoded "/" with Region.SEPARATOR
Posted by GitBox <gi...@apache.org>.
jujoramos edited a comment on pull request #5049:
URL: https://github.com/apache/geode/pull/5049#issuecomment-625698246
> My concern with this is that CacheConfig.SEPARATOR is a public field of a non-internal class, and as such removing it altogether might be a breaking change. Perhaps deprecating it and suggesting Region.SEPARATOR be used instead is the best compromise?
Good call, didn't realise `CacheConfig` is not part of an `internal` package (I was mistakenly thinking about the one in `org.apache.geode.internal.cache`)... I'm fine with deprecating the field, or just leaving it as it is to directly reference `Region.SEPARATOR`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jujoramos commented on a change in pull request #5049: GEODE-7864: Replace uses of hardcoded "/" with Region.SEPARATOR
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #5049:
URL: https://github.com/apache/geode/pull/5049#discussion_r421406231
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/InterestListEndpointDUnitTest.java
##########
@@ -284,7 +285,7 @@ public static void stopILEndpointServer() {
Iterator iter_prox = bs.getAcceptor().getCacheClientNotifier().getClientProxies().iterator();
if (iter_prox.hasNext()) {
CacheClientProxy proxy = (CacheClientProxy) iter_prox.next();
- // if (proxy._interestList._keysOfInterest.get("/"+REGION_NAME) != null) {
+ // if (proxy._interestList._keysOfInterest.get(SEPARATOR+REGION_NAME) != null) {
Review comment:
Maybe just delete this line as it's commented out?.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/InterestListEndpointDUnitTest.java
##########
@@ -398,9 +399,9 @@ public static void verifyIfNotInterestListEndpointAndThenPut() {
// only one server thats why if and not while
if (iter.hasNext()) {
CacheClientProxy proxy = (CacheClientProxy) iter.next();
- // if (proxy._interestList._keysOfInterest.get("/"+ REGION_NAME) == null) {
+ // if (proxy._interestList._keysOfInterest.get(SEPARATOR+ REGION_NAME) == null) {
Review comment:
Maybe just delete this line as it's commented out?.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/RegionCloseDUnitTest.java
##########
@@ -224,7 +225,7 @@ public String description() {
}
};
GeodeAwaitility.await().untilAsserted(ev);
- // assertNull(c.getRegion("/"+clientMembershipId));
+ // assertNull(c.getRegion(SEPARATOR+clientMembershipId));
Review comment:
Maybe just delete this line as it's commented out?.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -255,7 +257,8 @@ public void testScenario3() throws Exception {
assertEquals("get failed for corresponding put", "\"value1\"", tok);
current_fullpath = jtaObj.currRegion.getFullPath();
- assertEquals("failed retrieving current region fullpath", "/" + DEFAULT_RGN + "/region1",
+ assertEquals("failed retrieving current region fullpath",
+ SEPARATOR + DEFAULT_RGN + "/region1",
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -245,7 +246,8 @@ public void testScenario3() throws Exception {
jtaObj.getRegionFromCache("region1");
String current_fullpath = jtaObj.currRegion.getFullPath();
- assertEquals("failed retrieving current region fullpath", "/" + DEFAULT_RGN + "/region1",
+ assertEquals("failed retrieving current region fullpath",
+ SEPARATOR + DEFAULT_RGN + "/region1",
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -276,7 +279,7 @@ public void testScenario3() throws Exception {
current_fullpath = jtaObj.currRegion.getFullPath();
assertEquals("failed retrieving current region fullpath after txn commit",
- "/" + DEFAULT_RGN + "/region1", current_fullpath);
+ SEPARATOR + DEFAULT_RGN + "/region1", current_fullpath);
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/management/QueryDataDUnitTest.java
##########
@@ -313,7 +314,8 @@ public void testErrors() {
distributedSystemMXBean.queryData(regionsNotFoundOnMembersQuery, member1.getId(), 2);
assertThat(regionsNotFoundOnMembersResult, isJson(withJsonPath("$.message",
equalTo(
- String.format("Cannot find regions %s in specified members", "/" + regionName)))));
+ String.format("Cannot find regions %s in specified members",
+ SEPARATOR + regionName)))));
Review comment:
Scratch that, it's just a test...
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/management/QueryDataDUnitTest.java
##########
@@ -313,7 +314,8 @@ public void testErrors() {
distributedSystemMXBean.queryData(regionsNotFoundOnMembersQuery, member1.getId(), 2);
assertThat(regionsNotFoundOnMembersResult, isJson(withJsonPath("$.message",
equalTo(
- String.format("Cannot find regions %s in specified members", "/" + regionName)))));
+ String.format("Cannot find regions %s in specified members",
+ SEPARATOR + regionName)))));
Review comment:
Nitpick: since we're using `String.format` here, wouldn't it be better (performance wise) to include and extra parameter within the `String` instead of the concatenation?.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -341,7 +344,7 @@ public void testScenario4() throws Exception {
String current_fullpath = jtaObj.currRegion.getFullPath();
assertEquals(
"failed retrieving current region fullpath after doing getRegionFromCache(region1)",
- "/" + DEFAULT_RGN + "/region1", current_fullpath);
+ SEPARATOR + DEFAULT_RGN + "/region1", current_fullpath);
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -352,7 +355,8 @@ public void testScenario4() throws Exception {
assertEquals("get value do not match with the put", "\"value1\"", tok);
current_fullpath = jtaObj.currRegion.getFullPath();
- assertEquals("failed retrieving current region fullpath", "/" + DEFAULT_RGN + "/region1",
+ assertEquals("failed retrieving current region fullpath",
+ SEPARATOR + DEFAULT_RGN + "/region1",
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -454,7 +458,7 @@ public void testScenario5() throws Exception {
current_fullpath = jtaObj.currRegion.getFullPath();
assertEquals("failed retrieving current fullpath, current fullpath: " + current_fullpath,
- "/" + DEFAULT_RGN + "/region1", current_fullpath);
+ SEPARATOR + DEFAULT_RGN + "/region1", current_fullpath);
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -440,7 +444,7 @@ public void testScenario5() throws Exception {
// now current region should point to region1, as done from within
// getRegionFromCache method...
String current_fullpath = jtaObj.currRegion.getFullPath();
- assertEquals("failed retirieving current fullpath", "/" + DEFAULT_RGN + "/region1",
+ assertEquals("failed retirieving current fullpath", SEPARATOR + DEFAULT_RGN + "/region1",
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -372,7 +376,7 @@ public void testScenario4() throws Exception {
current_fullpath = jtaObj.currRegion.getFullPath();
assertEquals("failed retirieving current region fullpath after txn rollback",
- "/" + DEFAULT_RGN + "/region1", current_fullpath);
+ SEPARATOR + DEFAULT_RGN + "/region1", current_fullpath);
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -577,7 +582,8 @@ public void testScenario7() throws Exception {
assertEquals("get value mismatch with put", "\"value1\"", tok);
current_fullpath = jtaObj.currRegion.getFullPath();
- assertEquals("failed retrieving current region fullpath", "/" + DEFAULT_RGN + "/region1",
+ assertEquals("failed retrieving current region fullpath",
+ SEPARATOR + DEFAULT_RGN + "/region1",
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -590,7 +596,7 @@ public void testScenario7() throws Exception {
current_fullpath = jtaObj.currRegion.getFullPath();
assertEquals("failed retrieving current region fullpath after txn commit, fullpath is: "
- + current_region, "/" + DEFAULT_RGN + "/region1", current_fullpath);
+ + current_region, SEPARATOR + DEFAULT_RGN + "/region1", current_fullpath);
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/jta/functional/CacheJUnitTest.java
##########
@@ -505,7 +509,7 @@ public void testScenario5() throws Exception {
String current_fullpath = jtaObj.currRegion.getFullPath();
assertEquals(
"failed retrieving current fullpath after rollback, fullpath is: " + current_fullpath,
- "/" + DEFAULT_RGN + "/region1", current_fullpath);
+ SEPARATOR + DEFAULT_RGN + "/region1", current_fullpath);
Review comment:
Did you miss this one?, shouldn't it be `SEPARATOR + DEFAULT_RGN + SEPARATOR + "region1"`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jujoramos commented on pull request #5049: GEODE-7864: Replace uses of hardcoded "/" with Region.SEPARATOR
Posted by GitBox <gi...@apache.org>.
jujoramos commented on pull request #5049:
URL: https://github.com/apache/geode/pull/5049#issuecomment-625698246
> My concern with this is that CacheConfig.SEPARATOR is a public field of a non-internal class, and as such removing it altogether might be a breaking change. Perhaps deprecating it and suggesting Region.SEPARATOR be used instead is the best compromise?
Good call, didn't realise `CacheConfig` is not part of an `internal` package (I was mistakenly thinking about the one in `org.apache.geode.internal.cache`)... I'm fine with deprecating the fild, or just leaving it as it is to directly reference `Region.SEPARATOR`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on pull request #5049: GEODE-7864: Replace uses of hardcoded "/" with Region.SEPARATOR
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #5049:
URL: https://github.com/apache/geode/pull/5049#issuecomment-625553877
> Considering that the `CacheConfig.SEPARATOR` field is now _**only used within**_ the `CacheConfig` class and _**only in one single method**_, would it be feasible to directly remove it and always use `Region.SEPARATOR` across the code base?.
My concern with this is that `CacheConfig.SEPARATOR` is a public field of a non-internal class, and as such removing it altogether might be a breaking change. Perhaps deprecating it and suggesting `Region.SEPARATOR` be used instead is the best compromise?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on pull request #5049: GEODE-7864: Replace uses of hardcoded "/" with Region.SEPARATOR
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #5049:
URL: https://github.com/apache/geode/pull/5049#issuecomment-624910507
> easy way to test is try changing Region.SEPARATOR to something else, right?
This is just a first-pass, fixing uses like `getRegion("/" + regionName)`. It doesn't fix uses like `getRegion("/regionName")`. If you think it's better to take an all or nothing approach rather than incremental improvements, I'm okay with that too, but it might make proper review of the changes difficult. The last thing I want to do is replace a filepath separator with Region.SEPARATOR and have it slip through unnoticed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on pull request #5049: GEODE-7864: Replace uses of hardcoded "/" with Region.SEPARATOR
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #5049:
URL: https://github.com/apache/geode/pull/5049#issuecomment-624908788
easy way to test is try changing Region.SEPARATOR to something else, right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org