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/30 19:23:07 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #6702: GEODE-8870: Remove GFE_80
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