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/16 04:52:25 UTC
[GitHub] [geode] pivotal-jbarrett opened a new pull request #6702: GEODE-8870: Remove GFE_80
pivotal-jbarrett opened a new pull request #6702:
URL: https://github.com/apache/geode/pull/6702
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] lgtm-com[bot] commented on pull request #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6702:
URL: https://github.com/apache/geode/pull/6702#issuecomment-883756902
This pull request **introduces 1 alert** and **fixes 3** when merging e09eb2bf0d76bb6214d97365c9a253eafb05f7d2 into 0726d5a445d218b3775902d3a4dc4da5308ae4ed - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-4ceb3545d5cb47d1ca4597c6ee38d79412b2f013)
**new alerts:**
* 1 for Inefficient output stream
**fixed alerts:**
* 1 for Cast from abstract to concrete collection
* 1 for Potential output resource leak
* 1 for Dereferenced variable may be null
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680259788
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java
##########
@@ -87,7 +87,7 @@ public void incBreadcrumbCounter() {
/** The versions in which this message was modified */
@Immutable
- private static final KnownVersion[] dsfidVersions = new KnownVersion[] {KnownVersion.GFE_80};
+ private static final KnownVersion[] dsfidVersions = new KnownVersion[0];
Review comment:
eh... I went this route for two reasons.
1) We have been trending away from `null` to things like `Collections.emptyList()` so this `Object[0]` is effectively the same.
2) It feels strange having something that says it supports versions, by the existence of the interface, but return `null` when asked. An empty array/list felt better as a way to say, yes it is supported but there are no versions yet.
The larger change might be to remove the interface entirely from these classes but I wanted to keep the changes smaller.
On the other hand your argument for consistency is good. I would need to look. I don't recall seeing others that returned `null`, just that it defensively checked for null.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680270255
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java
##########
@@ -87,7 +87,7 @@ public void incBreadcrumbCounter() {
/** The versions in which this message was modified */
@Immutable
- private static final KnownVersion[] dsfidVersions = new KnownVersion[] {KnownVersion.GFE_80};
+ private static final KnownVersion[] dsfidVersions = new KnownVersion[0];
Review comment:
Given the push is towards `@NotNull` type behavior I think I am ok with this inconsistency as an example on how to proceed in the future with these classes.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680261767
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ObjectPartList.java
##########
@@ -42,8 +40,6 @@
* @since GemFire 5.7
*/
public class ObjectPartList implements DataSerializableFixedID, Releasable {
Review comment:
Oh... let me take a look.
--
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] echobravopapa commented on a change in pull request #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680175196
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -793,24 +784,12 @@ private static void writeBatchException(Message origMsg, List<BatchException70>
}
private static void writeFatalException(Message origMsg, Throwable exception,
- ServerConnection servConn, int batchId) throws IOException {
+ ServerConnection servConn) throws IOException {
Message errorMsg = servConn.getErrorResponseMessage();
errorMsg.setMessageType(MessageType.EXCEPTION);
errorMsg.setNumberOfParts(2);
errorMsg.setTransactionId(origMsg.getTransactionId());
-
- // For older gateway senders, we need to send back an exception
- // they can deserialize.
- if ((servConn.getClientVersion() == null
Review comment:
less code is better 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@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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680260255
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/ClientHealthStats.java
##########
@@ -314,10 +316,10 @@ public int getDSFID() {
}
public HashMap<String, String> getPoolStats() {
- return this.poolStats;
+ return poolStats;
}
public void setPoolStats(HashMap<String, String> statsMap) {
Review comment:
See comment above. In this case the change was automated removal of redundant `this` qualifications.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680270526
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -228,6 +224,7 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
long versionTimeStamp;
Part callbackArgExistsPart;
LocalRegion region;
+ Object callbackArg = null;
switch (actionType) {
case 0: // Create
try {
Review comment:
Ewww... yeah, what is that mess of code. Delete!
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r679456106
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java
##########
@@ -87,7 +87,7 @@ public void incBreadcrumbCounter() {
/** The versions in which this message was modified */
@Immutable
- private static final KnownVersion[] dsfidVersions = new KnownVersion[] {KnownVersion.GFE_80};
+ private static final KnownVersion[] dsfidVersions = new KnownVersion[0];
Review comment:
Looking at other classes that implement `DataSerializableFixedID`, it seems to be common to return `null` from `getSerializationVersions()` when no modifications have been made to serialization for the class rather than an empty array. Just for consistency, would it be better to do that here too? This also applies to several other classes touched in this PR.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -228,6 +224,7 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
long versionTimeStamp;
Part callbackArgExistsPart;
LocalRegion region;
+ Object callbackArg = null;
switch (actionType) {
case 0: // Create
try {
Review comment:
Below this line is a bunch of commented-out code which should probably be removed.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/ClientHealthStats.java
##########
@@ -91,7 +91,7 @@
/**
* Represents stats for a poolName .
**/
- private HashMap<String, String> poolStats = new HashMap<String, String>();
+ private HashMap<String, String> poolStats = new HashMap<>();
Review comment:
Could this instead be `Map<String, String> poolStats = new HashMap<>();`? It will require refactoring the `getPoolStats()` to return a `Map` but beyond that no other changes are needed.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -313,7 +309,7 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
isObject = putContext.isObject();
}
// Attempt to create the entry
- boolean result = false;
+ boolean result;
if (isPdxEvent) {
result = addPdxType(crHelper, key, value);
} else {
Review comment:
More commented-out code starting on line 350.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ObjectPartList.java
##########
@@ -42,8 +40,6 @@
* @since GemFire 5.7
*/
public class ObjectPartList implements DataSerializableFixedID, Releasable {
Review comment:
While looking at this class, I noticed that another class, `ObjectPartList651` is not used anywhere except to be extended by `SerializedObjectPartList`, which is also not used anywhere. I think that both these classes can probably be safely removed, particularly since both have comments at the top of them stating "THIS CLASS IS OBSOLETE AS OF V7.0"
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -685,11 +681,11 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
}
if (fatalException != null) {
serverConnection.incrementLatestBatchIdReplied(batchId);
- writeFatalException(clientMessage, fatalException, serverConnection, batchId);
+ writeFatalException(clientMessage, fatalException, serverConnection);
serverConnection.setAsTrue(RESPONDED);
} else if (!exceptions.isEmpty()) {
serverConnection.incrementLatestBatchIdReplied(batchId);
- writeBatchException(clientMessage, exceptions, serverConnection, batchId);
+ writeBatchException(clientMessage, exceptions, serverConnection);
serverConnection.setAsTrue(RESPONDED);
} else {
// Increment the batch id unless the received batch id is -1 (a failover
Review comment:
On line 732, the `Exception e` argument is not used within the `shouldThrowException()` method so can be removed from the signature. In fact, the entire `shouldThrowException()` is currently a no-op so could probably be removed.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -1824,16 +1816,12 @@ public void unlock(Object name) throws LockNotHeldException, LeaseExpiredExcepti
if (!hadRecursion && lockId > -1 && token != null) {
Review comment:
The `token != null` check here is always true, since there's a relationship between the values of `lockId` and `token`. Removing the null check might not be ideal, in case this code changes in future and we want to be sure that it's safe to proceed, but reordering the clauses to `!hadRecursion && token != null && lockId > -1` gets rid of the warning without leaving a gap in the logic.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -1435,10 +1429,10 @@ public boolean lockInterruptibly(final Object name, final long waitTimeMillis,
name, token);
}
reentrant = true;
- if (reentrant && disallowReentrant) {
+ if (disallowReentrant) {
throw new IllegalStateException(
String.format("%s attempted to reenter non-reentrant lock %s",
- new Object[] {Thread.currentThread(), token}));
+ Thread.currentThread(), token));
}
recursionBefore = token.getRecursion();
lockId = token.getLeaseId(); // keep lockId
Review comment:
On line 1449 we assert that `lockId > -1` if `reentrant` is true, but this assertion is always true. If `reentrant` is true, it means we hit line 1431, which in turn means that if `lockId` is less than 0 we restart the while loop without ever hitting this check because of the check on line 1439.
On line 1463 we set `reentrant` to false, but this is unnecessary as the value is never used again until we restart the loop, when it's immediately set to false anyway. This also happens on line 1509 (where we also set `recursionBefore` to -1 but never use that value).
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -2785,21 +2741,19 @@ public static boolean isLockGrantor(String serviceName) throws IllegalArgumentEx
* @param nonGrantors filled with service names of all services we have that we are not the
* grantor of.
*/
- public static void recoverRmtElder(ArrayList grantors, ArrayList grantorVersions,
- ArrayList grantorSerialNumbers, ArrayList nonGrantors) {
+ public static void recoverRmtElder(ArrayList<String> grantors, ArrayList<Long> grantorVersions,
Review comment:
Can these be `List<>` rather than `ArrayList<>`?
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java
##########
@@ -362,40 +359,40 @@ public void toData(DataOutput dop,
member.writeEssentialData(hdos);
DataSerializer.writeByteArray(hdos.toByteArray(), dop);
} else {
- DataSerializer.writeByteArray(this.membershipID, dop);
+ DataSerializer.writeByteArray(membershipID, dop);
}
- DataSerializer.writeByteArray(getOptimizedByteArrayForEventID(this.threadID, this.sequenceID),
+ DataSerializer.writeByteArray(getOptimizedByteArrayForEventID(threadID, sequenceID),
dop);
- dop.writeInt(this.bucketID);
- dop.writeByte(this.breadcrumbCounter);
+ dop.writeInt(bucketID);
+ dop.writeByte(breadcrumbCounter);
}
@Override
public void fromData(DataInput di,
DeserializationContext context) throws IOException, ClassNotFoundException {
- this.membershipID = DataSerializer.readByteArray(di);
+ membershipID = DataSerializer.readByteArray(di);
ByteBuffer eventIdParts = ByteBuffer.wrap(DataSerializer.readByteArray(di));
- this.threadID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
- this.sequenceID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
- this.bucketID = di.readInt();
- this.breadcrumbCounter = di.readByte();
+ threadID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
+ sequenceID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
+ bucketID = di.readInt();
+ breadcrumbCounter = di.readByte();
}
@Override
public void writeExternal(ObjectOutput out) throws IOException {
- DataSerializer.writeByteArray(this.membershipID, out);
- DataSerializer.writeByteArray(getOptimizedByteArrayForEventID(this.threadID, this.sequenceID),
+ DataSerializer.writeByteArray(membershipID, out);
+ DataSerializer.writeByteArray(getOptimizedByteArrayForEventID(threadID, sequenceID),
out);
- out.writeInt(this.bucketID);
+ out.writeInt(bucketID);
}
@Override
public void readExternal(ObjectInput in) throws IOException, ClassNotFoundException {
- this.membershipID = DataSerializer.readByteArray(in);
+ membershipID = DataSerializer.readByteArray(in);
ByteBuffer eventIdParts = ByteBuffer.wrap(DataSerializer.readByteArray(in));
- this.threadID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
- this.sequenceID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
- this.bucketID = in.readInt();
+ threadID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
+ sequenceID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
+ bucketID = in.readInt();
}
@Override
Review comment:
Typo on line 423, should be `MINIMUM_ID_LENGTH`.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/tcp/MsgStreamer.java
##########
@@ -378,28 +366,25 @@ public void close() throws IOException {
/** override OutputStream's write() */
@Override
- public void write(byte[] source, int offset, int len) {
- // if (logger.isTraceEnabled()) {
- // logger.trace(" bytes={} offset={} len={}", source, offset, len);
- // }
- if (this.overflowBuf != null) {
- this.overflowBuf.write(source, offset, len);
+ public void write(byte @NotNull [] source, int offset, int len) {
Review comment:
The placement of this `@NotNull` annotation seems a little odd. Should this instead be
```
write(@NotNull byte[] source, int offset, int len)
```
Also, if that reordering doesn't fix the false positive LGTM warning, it can be suppressed using
```
@SuppressWarnings("lgtm[java/inefficient-output-stream]")
```
##########
File path: geode-core/src/main/java/org/apache/geode/internal/tcp/MsgStreamer.java
##########
@@ -179,15 +171,13 @@ public static BaseMsgStreamer create(List<?> cons, final DistributionMessage msg
} else {
// if there is a versioned stream created, then split remaining
// connections to unversioned stream
- final ArrayList<MsgStreamer> streamers =
- new ArrayList<MsgStreamer>(versionToConnMap.size() + 1);
+ final ArrayList<MsgStreamer> streamers = new ArrayList<>(versionToConnMap.size() + 1);
Review comment:
Could this be a `List<>` instead?
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/ClientHealthStats.java
##########
@@ -314,10 +316,10 @@ public int getDSFID() {
}
public HashMap<String, String> getPoolStats() {
- return this.poolStats;
+ return poolStats;
}
public void setPoolStats(HashMap<String, String> statsMap) {
Review comment:
Could this method take a `Map<String, String>` instead?
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ObjectPartList.java
##########
@@ -56,48 +52,48 @@
protected boolean hasKeys;
- protected List keys;
+ protected List<Object> keys;
- protected List objects;
+ protected List<Object> objects;
public void addPart(Object key, Object value, byte objectType, VersionTag versionTag) {
Review comment:
Warnings here and on line 99 can be fixed by using `VersionTag<?>`
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -303,8 +300,7 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
handleMessageRetry(region, clientEvent);
Review comment:
On line 273 we're doing this:
```
versionTimeStamp = clientMessage.getPart(index++).getLong();
```
but then never using the incremented value of index. Either this increment is unnecessary and should be removed, or this is a bug and it's intended to be a pre increment.
On line 293 the compiler warning about raw use of parameterized class can be fixed by using:
```
VersionTag<?> tag = VersionTag.create(region.getVersionMember());
```
These comments also apply to the `case 1:`, `case 2:` and `case 3:` blocks.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -787,39 +783,40 @@ private void notLockGrantorId(LockGrantorId notLockGrantorId, long timeToWait,
if (logger.isTraceEnabled(LogMarker.DLS_VERBOSE)) {
Review comment:
The value of `timeUnit` is always `TimeUnit.MILLISECONDS` in this method, so this could be inlined and the javadoc and `timeToWait` argument modified to make it explicit that the time is in ms. If this is done, the same can be done to the `waitForLockGrantorFutureResult()` method.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -1016,27 +1013,27 @@ private void becomeLockGrantor(InternalDistributedMember predecessor) {
Review comment:
The `return` in the if statement immediately above this is redundant, since it's the last statement in a void method. The if statement can therefore be replaced with:
```
makeLocalGrantor(elder, needsRecovery, myLockGrantorId, myGrantor);
```
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/VersionedObjectList.java
##########
@@ -83,13 +81,13 @@ public void addKeyAndVersion(Object key, VersionTag versionTag) {
logger.trace(LogMarker.VERSIONED_OBJECT_LIST_VERBOSE,
Review comment:
There are many instances of raw use of the parameterized `VersionTag` class in this file that I think can be fixed by using `VersionTag<?>`.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -1095,12 +1092,12 @@ public void freeResources(Object name) {
* @return true if token has been destroyed and removed
*/
private boolean removeTokenIfUnused(Object name) {
- synchronized (this.tokens) {
- if (this.destroyed) {
+ synchronized (tokens) {
+ if (destroyed) {
getStats().incFreeResourcesFailed();
return false;
}
- DLockToken token = this.tokens.get(name);
+ DLockToken token = tokens.get(name);
if (token != null) {
synchronized (token) {
Review comment:
The IDE complains that since `token` is a local variable, it may be unsafe to synchronize on it. Given that we're already synchronized on the `tokens` field here, do we need to also synchronize on the element we get from that collection? I may well just be misunderstanding how synchronization works here. If this is something that could be improved, then we're doing the same thing in several other places in the class.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java
##########
@@ -362,40 +359,40 @@ public void toData(DataOutput dop,
member.writeEssentialData(hdos);
DataSerializer.writeByteArray(hdos.toByteArray(), dop);
} else {
- DataSerializer.writeByteArray(this.membershipID, dop);
+ DataSerializer.writeByteArray(membershipID, dop);
}
- DataSerializer.writeByteArray(getOptimizedByteArrayForEventID(this.threadID, this.sequenceID),
+ DataSerializer.writeByteArray(getOptimizedByteArrayForEventID(threadID, sequenceID),
dop);
- dop.writeInt(this.bucketID);
- dop.writeByte(this.breadcrumbCounter);
+ dop.writeInt(bucketID);
+ dop.writeByte(breadcrumbCounter);
}
@Override
public void fromData(DataInput di,
DeserializationContext context) throws IOException, ClassNotFoundException {
- this.membershipID = DataSerializer.readByteArray(di);
+ membershipID = DataSerializer.readByteArray(di);
ByteBuffer eventIdParts = ByteBuffer.wrap(DataSerializer.readByteArray(di));
- this.threadID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
- this.sequenceID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
- this.bucketID = di.readInt();
- this.breadcrumbCounter = di.readByte();
+ threadID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
+ sequenceID = readEventIdPartsFromOptmizedByteArray(eventIdParts);
+ bucketID = di.readInt();
+ breadcrumbCounter = di.readByte();
}
@Override
public void writeExternal(ObjectOutput out) throws IOException {
- DataSerializer.writeByteArray(this.membershipID, out);
- DataSerializer.writeByteArray(getOptimizedByteArrayForEventID(this.threadID, this.sequenceID),
+ DataSerializer.writeByteArray(membershipID, out);
+ DataSerializer.writeByteArray(getOptimizedByteArrayForEventID(threadID, sequenceID),
out);
- out.writeInt(this.bucketID);
+ out.writeInt(bucketID);
}
@Override
public void readExternal(ObjectInput in) throws IOException, ClassNotFoundException {
Review comment:
`ClassNotFoundException` is never thrown from here, so this can be removed.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/AbstractUpdateOperation.java
##########
@@ -85,19 +80,12 @@ protected void initMessage(CacheOperationMessage msg, DirectReplyProcessor pr) {
/** @return whether we should do a local create for a remote one */
private static boolean shouldDoRemoteCreate(LocalRegion rgn, EntryEventImpl ev) {
DataPolicy dp = rgn.getAttributes().getDataPolicy();
- if (!rgn.isAllEvents() || (dp.withReplication() && rgn.isInitialized()
- && ev.getOperation().isUpdate() && !rgn.getConcurrencyChecksEnabled()
- // misordered CREATE and
- // UPDATE messages can
- // cause inconsistencies
- && !ALWAYS_REPLICATE_UPDATES)) {
- // we are not accepting all events
- // or we are a replicate and initialized and it was an update
- // (we exclude that latter to avoid resurrecting a key deleted in a replicate
- return false;
- } else {
- return true;
- }
+ // we are not accepting all events or we are a replicate and initialized and it was an update
+ // (we exclude that latter to avoid resurrecting a key deleted in a replicate
+ // misordered CREATE and UPDATE messages can cause inconsistencies
+ return rgn.isAllEvents() && (!dp.withReplication() || !rgn.isInitialized()
+ || !ev.getOperation().isUpdate() || rgn.getConcurrencyChecksEnabled()
+ || ALWAYS_REPLICATE_UPDATES);
}
private static boolean checkIfToUpdateAfterCreateFailed(LocalRegion rgn, EntryEventImpl ev) {
Review comment:
It's a little inelegant, but the warning on line 99 can be fixed by using:
```
RegionVersionVector<InternalDistributedMember> versionVector =
uncheckedCast(rgn.getVersionVector());
versionVector.recordVersion((InternalDistributedMember) ev.getDistributedMember(),
uncheckedCast(ev.getVersionTag()));
```
this same approach can be taken on lines 209 and 235 to fix the warnings there.
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java
##########
@@ -114,7 +115,7 @@ protected void _distribute() {
@Test
public void testDoRemoveDestroyTokensFromCqResultKeys() {
Object key = new Object();
- HashMap hashMap = new HashMap();
+ HashMap<Long, Integer> hashMap = new HashMap<>();
Review comment:
Could this instead be a `Map<>`?
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680261086
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -303,8 +300,7 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
handleMessageRetry(region, clientEvent);
Review comment:
>On line 293 the compiler warning about raw use of parameterized class can be fixed by using
I have purposely stayed away from VersionTag generics. I went down that rabbit hole in other places and it is a mess. That type is a mess when it comes to generics. It relates to several other classes that are also a mess. Often times the wildcard capture results in all sorts of compile issues that then have to be resolved with more generic type matches and then ultimately some unchecked casts. I have a todo on my plate to try and fix this class.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680262720
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -1095,12 +1092,12 @@ public void freeResources(Object name) {
* @return true if token has been destroyed and removed
*/
private boolean removeTokenIfUnused(Object name) {
- synchronized (this.tokens) {
- if (this.destroyed) {
+ synchronized (tokens) {
+ if (destroyed) {
getStats().incFreeResourcesFailed();
return false;
}
- DLockToken token = this.tokens.get(name);
+ DLockToken token = tokens.get(name);
if (token != null) {
synchronized (token) {
Review comment:
I am not even going to try and address concurrency issues in simple cleanups. Nope. Never. Just look away.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680262013
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ObjectPartList.java
##########
@@ -56,48 +52,48 @@
protected boolean hasKeys;
- protected List keys;
+ protected List<Object> keys;
- protected List objects;
+ protected List<Object> objects;
public void addPart(Object key, Object value, byte objectType, VersionTag versionTag) {
Review comment:
See previous comments on the issues with `VersionTag`.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/VersionedObjectList.java
##########
@@ -83,13 +81,13 @@ public void addKeyAndVersion(Object key, VersionTag versionTag) {
logger.trace(LogMarker.VERSIONED_OBJECT_LIST_VERBOSE,
Review comment:
See previous comments on the issues with `VersionTag`.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680261476
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -303,8 +300,7 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
handleMessageRetry(region, clientEvent);
Review comment:
>On line 273 we're doing this:
I intentionally left that one in, and a few others similar to it, because it just felt more readable to increment as it goes down the parts collection. I can go either way with 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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6702:
URL: https://github.com/apache/geode/pull/6702#issuecomment-888695361
This pull request **introduces 1 alert** and **fixes 3** when merging 9d17457f8b3b02e4f47a322b99d9c173e5f8aef1 into 963d03a7ca8c21a95d65a152bc8ef118fa98ff10 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-2782a771570902d4a3ca69c9888f91aea5a38714)
**new alerts:**
* 1 for Inefficient output stream
**fixed alerts:**
* 1 for Cast from abstract to concrete collection
* 1 for Potential output resource leak
* 1 for Dereferenced variable may be null
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett merged pull request #6702:
URL: https://github.com/apache/geode/pull/6702
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680263094
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -1435,10 +1429,10 @@ public boolean lockInterruptibly(final Object name, final long waitTimeMillis,
name, token);
}
reentrant = true;
- if (reentrant && disallowReentrant) {
+ if (disallowReentrant) {
throw new IllegalStateException(
String.format("%s attempted to reenter non-reentrant lock %s",
- new Object[] {Thread.currentThread(), token}));
+ Thread.currentThread(), token));
}
recursionBefore = token.getRecursion();
lockId = token.getLeaseId(); // keep lockId
Review comment:
Again, not trying to address all warnings just most of the low hanging automated cleanups. Without sufficient testing in these areas I am going to tread very lightly on things that can affect readability and/or concurrency.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680262446
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -787,39 +783,40 @@ private void notLockGrantorId(LockGrantorId notLockGrantorId, long timeToWait,
if (logger.isTraceEnabled(LogMarker.DLS_VERBOSE)) {
Review comment:
It is intentional to avoid that trap. In the past we have been bad about changing units of time and not correcting the math at all the call points. While all the current usages always use milliseconds that may not be the case in the future. Time is hard and has bit us in the rear a few too many times.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett merged pull request #6702:
URL: https://github.com/apache/geode/pull/6702
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680304590
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java
##########
@@ -1016,27 +1013,27 @@ private void becomeLockGrantor(InternalDistributedMember predecessor) {
Review comment:
I am just going to steer clear of DLockService changes like this.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680260434
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -228,6 +224,7 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
long versionTimeStamp;
Part callbackArgExistsPart;
LocalRegion region;
+ Object callbackArg = null;
switch (actionType) {
case 0: // Create
try {
Review comment:
I will have a look. Many of these changes are just automated and I don't look at all the surrounding content.
--
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] lgtm-com[bot] commented on pull request #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6702:
URL: https://github.com/apache/geode/pull/6702#issuecomment-881189933
This pull request **introduces 1 alert** and **fixes 3** when merging d32443b1319b40dc73d66804979ce0870611d7b2 into 970497912dec5af43605842195bc8a7dd1479851 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-36cf7caaaf3365eefba48f10b0fa57a1f4235f8e)
**new alerts:**
* 1 for Inefficient output stream
**fixed alerts:**
* 1 for Cast from abstract to concrete collection
* 1 for Potential output resource leak
* 1 for Dereferenced variable may be null
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680304405
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ObjectPartList.java
##########
@@ -42,8 +40,6 @@
* @since GemFire 5.7
*/
public class ObjectPartList implements DataSerializableFixedID, Releasable {
Review comment:
Sure looks like dead code. Delete.
--
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] lgtm-com[bot] commented on pull request #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6702:
URL: https://github.com/apache/geode/pull/6702#issuecomment-881532890
This pull request **introduces 1 alert** and **fixes 3** when merging eb3db45b1fe91ab23b865347c87e56357e0d7819 into 970497912dec5af43605842195bc8a7dd1479851 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-ba42479e10cbd57765f18cd7f7abc204198ac327)
**new alerts:**
* 1 for Inefficient output stream
**fixed alerts:**
* 1 for Cast from abstract to concrete collection
* 1 for Potential output resource leak
* 1 for Dereferenced variable may be null
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680264901
##########
File path: geode-core/src/main/java/org/apache/geode/internal/tcp/MsgStreamer.java
##########
@@ -378,28 +366,25 @@ public void close() throws IOException {
/** override OutputStream's write() */
@Override
- public void write(byte[] source, int offset, int len) {
- // if (logger.isTraceEnabled()) {
- // logger.trace(" bytes={} offset={} len={}", source, offset, len);
- // }
- if (this.overflowBuf != null) {
- this.overflowBuf.write(source, offset, len);
+ public void write(byte @NotNull [] source, int offset, int len) {
Review comment:
It is a bug in LGTM. An issue was opened a year ago that they haven't resolved yet. I like the annotation though.
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r680260081
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/ClientHealthStats.java
##########
@@ -91,7 +91,7 @@
/**
* Represents stats for a poolName .
**/
- private HashMap<String, String> poolStats = new HashMap<String, String>();
+ private HashMap<String, String> poolStats = new HashMap<>();
Review comment:
It could be. I wasn't trying to get that invasive to use interfaces over implementations. In this case this was an automated change from cleaning up the redundant use of generic type specifiers.
--
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] lgtm-com[bot] commented on pull request #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6702:
URL: https://github.com/apache/geode/pull/6702#issuecomment-885091320
This pull request **introduces 1 alert** and **fixes 3** when merging e943ccc5ad2ba81f1c9b4599f130ea9fdcd23567 into 4338015abc1605217204681f5043c3bb6f1c714a - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-e8c423efca1afc09d7912beab1cf3816822eb9f8)
**new alerts:**
* 1 for Inefficient output stream
**fixed alerts:**
* 1 for Cast from abstract to concrete collection
* 1 for Potential output resource leak
* 1 for Dereferenced variable may be null
--
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] lgtm-com[bot] commented on pull request #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6702:
URL: https://github.com/apache/geode/pull/6702#issuecomment-890294071
This pull request **introduces 1 alert** and **fixes 3** when merging dc1fcd6f57632499c90c156c79b83413d4f18d81 into 138d5b8969a70bde07ad729461f1d914aa0b14f8 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-47500e6d46ffff79a4910cf6b85e53d0decbb4bf)
**new alerts:**
* 1 for Inefficient output stream
**fixed alerts:**
* 1 for Cast from abstract to concrete collection
* 1 for Potential output resource leak
* 1 for Dereferenced variable may be null
--
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] echobravopapa commented on a change in pull request #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r679323692
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -793,24 +784,12 @@ private static void writeBatchException(Message origMsg, List<BatchException70>
}
private static void writeFatalException(Message origMsg, Throwable exception,
- ServerConnection servConn, int batchId) throws IOException {
+ ServerConnection servConn) throws IOException {
Message errorMsg = servConn.getErrorResponseMessage();
errorMsg.setMessageType(MessageType.EXCEPTION);
errorMsg.setNumberOfParts(2);
errorMsg.setTransactionId(origMsg.getTransactionId());
-
- // For older gateway senders, we need to send back an exception
- // they can deserialize.
- if ((servConn.getClientVersion() == null
Review comment:
what about the `null` case?
--
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 #6702: GEODE-8870: Remove GFE_80
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6702:
URL: https://github.com/apache/geode/pull/6702#discussion_r679334897
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GatewayReceiverCommand.java
##########
@@ -793,24 +784,12 @@ private static void writeBatchException(Message origMsg, List<BatchException70>
}
private static void writeFatalException(Message origMsg, Throwable exception,
- ServerConnection servConn, int batchId) throws IOException {
+ ServerConnection servConn) throws IOException {
Message errorMsg = servConn.getErrorResponseMessage();
errorMsg.setMessageType(MessageType.EXCEPTION);
errorMsg.setNumberOfParts(2);
errorMsg.setTransactionId(origMsg.getTransactionId());
-
- // For older gateway senders, we need to send back an exception
- // they can deserialize.
- if ((servConn.getClientVersion() == null
Review comment:
As it turns out it can never be `null` from `ServerConnection.getClientVersion()`. The implementation would explode with exceptions if it were. Would you like me to annotate `@NotNull` through this path?
--
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