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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
