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/07/13 03:31:22 UTC
[GitHub] [geode] pivotal-jbarrett opened a new pull request #6697: GEODE-8870: Removes GFE_71
pivotal-jbarrett opened a new pull request #6697:
URL: https://github.com/apache/geode/pull/6697
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r678541285
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -235,23 +232,19 @@ public boolean hasUUID() {
}
public int compare(MemberIdentifier other) {
- return this.compareTo(other, false, true);
+ return compareTo(other, false, true);
}
@Override
- public int compareTo(MemberIdentifier other, boolean compareMemberData,
- boolean compareViewIds) {
- int myPort = getMembershipPort();
- int otherPort = other.getMembershipPort();
- if (myPort < otherPort) {
- return -1;
- }
- if (myPort > otherPort) {
- return 1;
+ public int compareTo(final @NotNull MemberIdentifier other, final boolean compareMemberData,
+ final boolean compareViewIds) {
+ int c = Integer.compare(getMembershipPort(), other.getMembershipPort());
Review comment:
Yeah, I toyed with that but it didn't really make it more readable in my opinion so I backed it out. I would just like to see us go to Java 9 or newer where most of these are replaced with built-ins.
--
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] pivotal-jbarrett commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r670679112
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/FilterRoutingInfo.java
##########
@@ -231,16 +209,16 @@ public void fromData(DataInput in) throws IOException, ClassNotFoundException {
InternalDataSerializer.invokeFromData(fInfo, in);
Review comment:
Any call that pulls the DS or Cache out of the ether should be deprecated. I am not scoping this these cleanups to try and resolve all of that at this time. There is a very old Geode addressing that issue.
--
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] Bill commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r678523007
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -282,49 +275,36 @@ public int compareTo(MemberIdentifier other, boolean compareMemberData,
}
}
- String myName = getName();
- String otherName = other.getName();
- if (!(other.isPartial() || this.isPartial())) {
- if (myName == null && otherName == null) {
- // do nothing
- } else if (myName == null) {
- return -1;
- } else if (otherName == null) {
- return 1;
- } else {
- int i = myName.compareTo(otherName);
- if (i != 0) {
- return i;
- }
+ if (!(other.isPartial() || isPartial())) {
+ c = StringUtils.compare(getName(), other.getName());
Review comment:
bravissimo!
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -235,23 +232,19 @@ public boolean hasUUID() {
}
public int compare(MemberIdentifier other) {
- return this.compareTo(other, false, true);
+ return compareTo(other, false, true);
}
@Override
- public int compareTo(MemberIdentifier other, boolean compareMemberData,
- boolean compareViewIds) {
- int myPort = getMembershipPort();
- int otherPort = other.getMembershipPort();
- if (myPort < otherPort) {
- return -1;
- }
- if (myPort > otherPort) {
- return 1;
+ public int compareTo(final @NotNull MemberIdentifier other, final boolean compareMemberData,
+ final boolean compareViewIds) {
+ int c = Integer.compare(getMembershipPort(), other.getMembershipPort());
Review comment:
I see why you made `c` mutable—so that you could use it to capture the result of subsequent comparisons below.
If you wanted to make it `final` and make new `final` variables that would be ok too.
--
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] pivotal-jbarrett commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r671420806
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -284,7 +279,7 @@ public int compareTo(MemberIdentifier other, boolean compareMemberData,
String myName = getName();
String otherName = other.getName();
- if (!(other.isPartial() || this.isPartial())) {
+ if (!(other.isPartial() || isPartial())) {
if (myName == null && otherName == null) {
Review comment:
I really wish we were at least Java 9 already. There are array comparators in 9. Some day... hopefully before I retire.
--
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] nabarunnag commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r678542922
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -118,7 +115,7 @@ public short getVersionOrdinal() {
* Returns the port on which the direct channel runs
*/
public int getDirectChannelPort() {
- assert !this.isPartial();
+ assert !isPartial();
Review comment:
This was removed in InternalDistributedMember but not here.
--
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] pivotal-jbarrett commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r678571132
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
##########
@@ -272,10 +271,8 @@ public DurableClientAttributes getDurableClientAttributes() {
return durableClientAttributes;
}
- /**
- * Returns an unmodifiable Set of this member's Roles.
- */
@Override
+ @Deprecated
Review comment:
The deprecation is documented in the interface this method derives from. Unfortunately the author did not provide an alternative. Best I can assume from the documentation is that the concept is unused and there is no replacement.
--
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] pivotal-jbarrett commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r670679545
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/FilterRoutingInfo.java
##########
@@ -386,121 +330,121 @@ public void toData(DataOutput out) throws IOException {
byte[] myData = StaticSerialization.getThreadLocalByteArray(size);
hdos = new HeapDataOutputStream(myData);
hdos.disallowExpansion();
- if (this.cqs == null) {
+ if (cqs == null) {
hdos.writeBoolean(false);
} else {
hdos.writeBoolean(true);
InternalDataSerializer.writeArrayLength(cqs.size(), hdos);
- for (Iterator it = this.cqs.entrySet().iterator(); it.hasNext();) {
- Map.Entry e = (Map.Entry) it.next();
+ for (final Map.Entry<Long, Integer> longIntegerEntry : cqs.entrySet()) {
// most cq IDs and all event types are small ints, so we use an optimized
// write that serializes 7 bits at a time in a compact form
- InternalDataSerializer.writeUnsignedVL((Long) e.getKey(), hdos);
- InternalDataSerializer.writeUnsignedVL((Integer) e.getValue(), hdos);
+ InternalDataSerializer.writeUnsignedVL(longIntegerEntry.getKey(), hdos);
+ InternalDataSerializer.writeUnsignedVL(longIntegerEntry.getValue(), hdos);
}
}
- InternalDataSerializer.writeSetOfLongs(this.interestedClients, this.longIDs, hdos);
- InternalDataSerializer.writeSetOfLongs(this.interestedClientsInv, this.longIDs, hdos);
+ InternalDataSerializer.writeSetOfLongs(interestedClients, longIDs, hdos);
+ InternalDataSerializer.writeSetOfLongs(interestedClientsInv, longIDs, hdos);
Review comment:
There is a PR pending that removes 8.0.
--
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] pivotal-jbarrett commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r670960973
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -284,7 +279,7 @@ public int compareTo(MemberIdentifier other, boolean compareMemberData,
String myName = getName();
String otherName = other.getName();
- if (!(other.isPartial() || this.isPartial())) {
+ if (!(other.isPartial() || isPartial())) {
if (myName == null && otherName == null) {
Review comment:
A bit of a rabbit hole but I like it better now.
--
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] pivotal-jbarrett commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r670678326
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/FilterRoutingInfo.java
##########
@@ -509,22 +453,22 @@ private void deserialize() {
@Override
public String toString() {
StringBuilder sb = new StringBuilder();
- if (this.interestedClients != null && this.interestedClients.size() > 0) {
+ if (interestedClients != null && interestedClients.size() > 0) {
sb.append("interestedClients:");
- sb.append(this.interestedClients);
+ sb.append(interestedClients);
}
- if (this.interestedClientsInv != null && this.interestedClientsInv.size() > 0) {
+ if (interestedClientsInv != null && interestedClientsInv.size() > 0) {
sb.append(", interestedClientsInv:");
- sb.append(this.interestedClientsInv);
+ sb.append(interestedClientsInv);
}
if (InternalDistributedSystem.getLogger().finerEnabled()) {
Review comment:
I am staying away from the legacy logger stuff.
--
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] pivotal-jbarrett merged pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett merged pull request #6697:
URL: https://github.com/apache/geode/pull/6697
--
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] DonalEvans commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r671364327
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -284,7 +279,7 @@ public int compareTo(MemberIdentifier other, boolean compareMemberData,
String myName = getName();
String otherName = other.getName();
- if (!(other.isPartial() || this.isPartial())) {
+ if (!(other.isPartial() || isPartial())) {
if (myName == null && otherName == null) {
Review comment:
Oh yeah, that's waaaay better now, nice.
--
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] nabarunnag commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r678543376
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
##########
@@ -272,10 +271,8 @@ public DurableClientAttributes getDurableClientAttributes() {
return durableClientAttributes;
}
- /**
- * Returns an unmodifiable Set of this member's Roles.
- */
@Override
+ @Deprecated
Review comment:
A comment will be helpful mentioning what alternative to use instead of the deprecated
--
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] pivotal-jbarrett commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r670872385
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -284,7 +279,7 @@ public int compareTo(MemberIdentifier other, boolean compareMemberData,
String myName = getName();
String otherName = other.getName();
- if (!(other.isPartial() || this.isPartial())) {
+ if (!(other.isPartial() || isPartial())) {
if (myName == null && otherName == null) {
Review comment:
I want to. I really do. I was worried about the readability change in similar empty blocks like this. Let me noodle it and see if refactoring it changes the readability.
--
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] DonalEvans commented on a change in pull request #6697: GEODE-8870: Removes GFE_71
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6697:
URL: https://github.com/apache/geode/pull/6697#discussion_r670614514
##########
File path: geode-membership/src/integrationTest/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt
##########
@@ -4,10 +4,8 @@ toData,72
org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl,6
Review comment:
This 6 needs to be a 4 now that two of the methods below have been removed.
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -824,61 +765,6 @@ public void fromDataPre_GFE_9_0_0_0(DataInput in, DeserializationContext context
// Assert.assertTrue(getPort() > 0);
Review comment:
Can this commented-out code be removed?
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/FilterRoutingInfo.java
##########
@@ -509,22 +453,22 @@ private void deserialize() {
@Override
public String toString() {
StringBuilder sb = new StringBuilder();
- if (this.interestedClients != null && this.interestedClients.size() > 0) {
+ if (interestedClients != null && interestedClients.size() > 0) {
sb.append("interestedClients:");
- sb.append(this.interestedClients);
+ sb.append(interestedClients);
}
- if (this.interestedClientsInv != null && this.interestedClientsInv.size() > 0) {
+ if (interestedClientsInv != null && interestedClientsInv.size() > 0) {
sb.append(", interestedClientsInv:");
- sb.append(this.interestedClientsInv);
+ sb.append(interestedClientsInv);
}
if (InternalDistributedSystem.getLogger().finerEnabled()) {
Review comment:
This method is deprecated. It might be better to replace it with:
```
LogService.getLogger().isTraceEnabled()
```
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/FilterRoutingInfo.java
##########
@@ -386,121 +330,121 @@ public void toData(DataOutput out) throws IOException {
byte[] myData = StaticSerialization.getThreadLocalByteArray(size);
hdos = new HeapDataOutputStream(myData);
hdos.disallowExpansion();
- if (this.cqs == null) {
+ if (cqs == null) {
hdos.writeBoolean(false);
} else {
hdos.writeBoolean(true);
InternalDataSerializer.writeArrayLength(cqs.size(), hdos);
- for (Iterator it = this.cqs.entrySet().iterator(); it.hasNext();) {
- Map.Entry e = (Map.Entry) it.next();
+ for (final Map.Entry<Long, Integer> longIntegerEntry : cqs.entrySet()) {
// most cq IDs and all event types are small ints, so we use an optimized
// write that serializes 7 bits at a time in a compact form
- InternalDataSerializer.writeUnsignedVL((Long) e.getKey(), hdos);
- InternalDataSerializer.writeUnsignedVL((Integer) e.getValue(), hdos);
+ InternalDataSerializer.writeUnsignedVL(longIntegerEntry.getKey(), hdos);
+ InternalDataSerializer.writeUnsignedVL(longIntegerEntry.getValue(), hdos);
}
}
- InternalDataSerializer.writeSetOfLongs(this.interestedClients, this.longIDs, hdos);
- InternalDataSerializer.writeSetOfLongs(this.interestedClientsInv, this.longIDs, hdos);
+ InternalDataSerializer.writeSetOfLongs(interestedClients, longIDs, hdos);
+ InternalDataSerializer.writeSetOfLongs(interestedClientsInv, longIDs, hdos);
Review comment:
This code block is duplicated in `toDataPre_GFE_8_0_0_0()`. Could it be pulled out into a method, maybe something like "writeCQData()"? If the `toDataPre_GFE_8_0_0_0()` method is shortly to be removed as well though, then there's no need to bother with this.
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -284,7 +279,7 @@ public int compareTo(MemberIdentifier other, boolean compareMemberData,
String myName = getName();
String otherName = other.getName();
- if (!(other.isPartial() || this.isPartial())) {
+ if (!(other.isPartial() || isPartial())) {
if (myName == null && otherName == null) {
Review comment:
The warning about an empty if statement here can be fixed by refactoring the block to:
```
if (myName != null || otherName != null) {
if (myName == null) {
return -1;
} else if (otherName == null) {
return 1;
} else {
int i = myName.compareTo(otherName);
if (i != 0) {
return i;
}
}
}
```
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/FilterRoutingInfo.java
##########
@@ -231,16 +209,16 @@ public void fromData(DataInput in) throws IOException, ClassNotFoundException {
InternalDataSerializer.invokeFromData(fInfo, in);
Review comment:
Further up in this method there's a call to `GemFireCacheImpl.getInstance()`, which is deprecated. Would it be possible to replace it with a non-deprecated call, possibly:
```
InternalDistributedSystem.getConnectedInstance().getCache();
```
--
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