You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by do...@apache.org on 2020/08/27 21:16:48 UTC
[geode] branch develop updated: GEODE-7864: Fix some LGTM alerts
and suppress some false positives (#5473)
This is an automated email from the ASF dual-hosted git repository.
donalevans pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new 93efb80 GEODE-7864: Fix some LGTM alerts and suppress some false positives (#5473)
93efb80 is described below
commit 93efb8051f8f6803d610fe86a8c68c8bfd18a27c
Author: Donal Evans <do...@pivotal.io>
AuthorDate: Thu Aug 27 14:16:11 2020 -0700
GEODE-7864: Fix some LGTM alerts and suppress some false positives (#5473)
- Fix dereferenced variable may be null alerts in Oplog, PartitionedRegion, InitialImageOperation, GMSMembership, ParallelGatewaySenderQueue, DiskStoreImpl, LocalRegion and HARegionQueue
- Remove redundant check in Oplog.java
- Suppress false positive alerts in SessionCachingFilter, NullToken, Undefined, ClusterDistributionManager and LocalRegion
- Add explanatory comments for LGTM suppressions
Authored-by: Donal Evans <do...@vmware.com>
---
.../session/filter/SessionCachingFilter.java | 4 +-
.../geode/cache/query/internal/NullToken.java | 3 +
.../geode/cache/query/internal/Undefined.java | 7 +-
.../internal/ClusterDistributionManager.java | 5 +-
.../apache/geode/internal/cache/DiskStoreImpl.java | 2 +-
.../internal/cache/InitialImageOperation.java | 43 ++--
.../apache/geode/internal/cache/LocalRegion.java | 19 +-
.../org/apache/geode/internal/cache/Oplog.java | 243 +++++++++++----------
.../geode/internal/cache/PartitionedRegion.java | 26 +--
.../geode/internal/cache/ha/HARegionQueue.java | 2 +-
.../internal/cache/partitioned/DumpB2NRegion.java | 4 +-
.../wan/parallel/ParallelGatewaySenderQueue.java | 11 +-
.../internal/security/MBeanServerWrapper.java | 3 +-
.../internal/cache/AbstractRegionJUnitTest.java | 4 +-
.../internal/membership/gms/GMSMembership.java | 3 +-
15 files changed, 195 insertions(+), 184 deletions(-)
diff --git a/extensions/geode-modules-session/src/main/java/org/apache/geode/modules/session/filter/SessionCachingFilter.java b/extensions/geode-modules-session/src/main/java/org/apache/geode/modules/session/filter/SessionCachingFilter.java
index b137ad0..ff171d8 100644
--- a/extensions/geode-modules-session/src/main/java/org/apache/geode/modules/session/filter/SessionCachingFilter.java
+++ b/extensions/geode-modules-session/src/main/java/org/apache/geode/modules/session/filter/SessionCachingFilter.java
@@ -209,7 +209,9 @@ public class SessionCachingFilter implements Filter {
cookie.setPath("".equals(getContextPath()) ? "/" : getContextPath());
cookie.setHttpOnly(cookieConfig.isHttpOnly());
cookie.setSecure(cookieConfig.isSecure());
- response.addCookie(cookie);
+ response.addCookie(cookie); // lgtm [java/insecure-cookie]
+ // The lgtm alert for the above line is suppressed as a false positive, since we are simply
+ // using the security setting from the context's cookie config
}
/**
diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/NullToken.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/NullToken.java
index fe007c1..2f39719 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/NullToken.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/NullToken.java
@@ -33,6 +33,9 @@ import org.apache.geode.internal.serialization.SerializationContext;
*/
public class NullToken implements DataSerializableFixedID, Comparable {
+ @SuppressWarnings("lgtm[java/useless-null-check]")
+ // The "useless null check" alert is suppressed here as the first time the constructor is called,
+ // QueryService.NULL is actually null, but subsequent times it is not
public NullToken() {
Support.assertState(IndexManager.NULL == null, "NULL constant already instantiated");
}
diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/Undefined.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/Undefined.java
index 26c2df5..7998fcc 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/Undefined.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/Undefined.java
@@ -40,12 +40,11 @@ public class Undefined implements DataSerializableFixedID, Comparable, Serializa
private static final long serialVersionUID = 6643107525908324141L;
+ @SuppressWarnings("lgtm[java/useless-null-check]")
+ // The "useless null check" alert is suppressed here as the first time the constructor is called,
+ // QueryService.UNDEFINED is actually null, but subsequent times it is not
public Undefined() {
Support.assertState(QueryService.UNDEFINED == null, "UNDEFINED constant already instantiated");
-
- // org.apache.persistence.CanonicalizationHelper
- // .putCanonicalObj("com/gemstone/persistence/query/QueryService.UNDEFINED",
- // this);
}
@Override
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
index 1219420..a2af201 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
@@ -1380,13 +1380,16 @@ public class ClusterDistributionManager implements DistributionManager {
/**
* This stalls waiting for the current membership view (as seen by the membership manager) to be
- * acknowledged by all membership listeners
+ * acknowledged by all membership listeners.
*/
+ @SuppressWarnings("lgtm[java/constant-comparison]")
void waitForViewInstallation(long id) throws InterruptedException {
if (id <= membershipViewIdAcknowledged) {
return;
}
synchronized (membershipViewIdGuard) {
+ // The LGTM alert for this line is suppressed as it fails to take into account the value of
+ // membershipViewIdAcknowledged being modified by another thread
while (membershipViewIdAcknowledged < id && !stopper.isCancelInProgress()) {
if (logger.isDebugEnabled()) {
logger.debug("waiting for view {}. Current DM view processed by all listeners is {}", id,
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
index 718dd7e..8d71134 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
@@ -837,7 +837,7 @@ public class DiskStoreImpl implements DiskStore {
if (logger.isDebugEnabled()) {
logger.debug(
"DiskRegion: Tried {}, getBytesAndBitsWithoutLock returns wrong byte array: {}",
- count, Arrays.toString(bb.getBytes()));
+ count, Arrays.toString(bb != null ? bb.getBytes() : null));
}
ex = e;
}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java b/geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
index 3698dbb..3278a3e 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
@@ -378,7 +378,7 @@ public class InitialImageOperation {
}
Boolean inhibitFlush = (Boolean) inhibitStateFlush.get();
- if (!inhibitFlush.booleanValue() && !this.region.doesNotDistribute()) {
+ if (!inhibitFlush && !this.region.doesNotDistribute()) {
if (region instanceof BucketRegionQueue) {
// get the corresponding userPRs and do state flush on all of them
// TODO we should be able to do this state flush with a single
@@ -457,7 +457,8 @@ public class InitialImageOperation {
this.region.getFullPath());
}
m.versionVector = null;
- } else if (keysOfUnfinishedOps.size() > MAXIMUM_UNFINISHED_OPERATIONS) {
+ } else if (keysOfUnfinishedOps != null
+ && keysOfUnfinishedOps.size() > MAXIMUM_UNFINISHED_OPERATIONS) {
if (isDebugEnabled) {
logger.debug(
"Region {} has {} unfinished operations, which exceeded threshold {}, do full GII instead",
@@ -843,11 +844,8 @@ public class InitialImageOperation {
if (entryCount <= 1000 && isDebugEnabled) {
keys = new HashSet();
}
- final ByteArrayDataInput in = new ByteArrayDataInput();
- List<Entry> entriesToSynchronize = null;
- if (this.isSynchronizing) {
- entriesToSynchronize = new ArrayList<>();
- }
+ List<Entry> entriesToSynchronize = new ArrayList<>();
+
for (int i = 0; i < entryCount; i++) {
// stream is null-terminated
if (internalDuringApplyDelta != null && !internalDuringApplyDelta.isRunning
@@ -891,13 +889,6 @@ public class InitialImageOperation {
final long lastModified = entry.getLastModified(this.region.getDistributionManager());
Object tmpValue = entry.value;
- byte[] tmpBytes = null;
-
- {
- if (tmpValue instanceof byte[]) {
- tmpBytes = (byte[]) tmpValue;
- }
- }
boolean didIIP = false;
boolean wasRecovered = false;
@@ -939,7 +930,7 @@ public class InitialImageOperation {
tag.replaceNullIDs(sender);
}
boolean record;
- if (this.region.getVersionVector() != null) {
+ if (this.region.getVersionVector() != null && tag != null) {
this.region.getVersionVector().recordVersion(tag.getMemberID(), tag);
record = true;
} else {
@@ -953,9 +944,7 @@ public class InitialImageOperation {
entriesToSynchronize.add(entry);
}
}
- } catch (RegionDestroyedException e) {
- return false;
- } catch (CancelException e) {
+ } catch (RegionDestroyedException | CancelException e) {
return false;
}
didIIP = true;
@@ -990,7 +979,7 @@ public class InitialImageOperation {
"processChunk:initialImagePut:key={},lastModified={},tmpValue={},wasRecovered={},tag={}",
entry.key, lastModified, tmpValue, wasRecovered, tag);
}
- if (this.region.getVersionVector() != null) {
+ if (this.region.getVersionVector() != null && tag != null) {
this.region.getVersionVector().recordVersion(tag.getMemberID(), tag);
}
this.entries.initialImagePut(entry.key, lastModified, tmpValue, wasRecovered, false,
@@ -998,12 +987,9 @@ public class InitialImageOperation {
if (this.isSynchronizing) {
entriesToSynchronize.add(entry);
}
- } catch (RegionDestroyedException e) {
- return false;
- } catch (CancelException e) {
+ } catch (RegionDestroyedException | CancelException e) {
return false;
}
- didIIP = true;
}
}
if (this.isSynchronizing && !entriesToSynchronize.isEmpty()) {
@@ -1724,7 +1710,8 @@ public class InitialImageOperation {
if (isGiiDebugEnabled) {
RegionVersionHolder holderOfRequest =
this.versionVector.getHolderForMember(this.lostMemberVersionID);
- if (holderToSync.isNewerThanOrCanFillExceptionsFor(holderOfRequest)) {
+ if (holderToSync != null
+ && holderToSync.isNewerThanOrCanFillExceptionsFor(holderOfRequest)) {
logger.trace(LogMarker.INITIAL_IMAGE_VERBOSE,
"synchronizeWith detected mismatch region version holder for lost member {}. Old is {}, new is {}",
lostMemberVersionID, holderOfRequest, holderToSync);
@@ -2445,9 +2432,11 @@ public class InitialImageOperation {
} finally {
if (received_rvv == null) {
if (isGiiDebugEnabled) {
- logger.trace(LogMarker.INITIAL_IMAGE_VERBOSE,
- "{} did not send back rvv. Maybe it's non-persistent proxy region or remote region {} not found or not initialized. Nothing to do.",
- reply.getSender(), region.getFullPath());
+ if (reply != null) {
+ logger.trace(LogMarker.INITIAL_IMAGE_VERBOSE,
+ "{} did not send back rvv. Maybe it's non-persistent proxy region or remote region {} not found or not initialized. Nothing to do.",
+ reply.getSender(), region.getFullPath());
+ }
}
}
super.process(msg);
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
index 257fa23..c5ce44c 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
@@ -3049,11 +3049,9 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
if (requireOldValue && result == null) {
throw new EntryNotFoundException("entry not found for replace");
}
- if (!requireOldValue) {
- if (!(Boolean) result) {
- // customers don't see this exception
- throw new EntryNotFoundException("entry found with wrong value");
- }
+ if (!requireOldValue && result != null && !((Boolean) result)) {
+ // customers don't see this exception
+ throw new EntryNotFoundException("entry found with wrong value");
}
}
}
@@ -3369,11 +3367,10 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
RegionEventImpl regionEvent =
new RegionEventImpl(this, Operation.REGION_DESTROY, null, true, getMyId());
regionEvent.setEventID(eventID);
- FilterInfo clientRouting = routing;
- if (clientRouting == null) {
- clientRouting = fp.getLocalFilterRouting(regionEvent);
+ if (routing == null) {
+ routing = fp.getLocalFilterRouting(regionEvent);
}
- regionEvent.setLocalFilterInfo(clientRouting);
+ regionEvent.setLocalFilterInfo(routing);
ClientUpdateMessage clientMessage =
ClientTombstoneMessage.gc(this, regionGCVersions, eventID);
CacheClientNotifier.notifyClients(regionEvent, clientMessage);
@@ -3450,7 +3447,9 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
checkEntryNotFound(keyInfo.getKey());
}
// OFFHEAP returned to callers
- Object value = regionEntry.getValueInVM(this);
+ // The warning for regionEntry possibly being null is incorrect, as checkEntryNotFound() always
+ // throws, making the following line unreachable if regionEntry is null
+ Object value = regionEntry.getValueInVM(this); // lgtm [java/dereferenced-value-may-be-null]
if (Token.isRemoved(value)) {
checkEntryNotFound(keyInfo.getKey());
}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java b/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
index 216644f..4a7b255 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
@@ -1748,29 +1748,32 @@ public class Oplog implements CompactableOplog, Flushable {
oplogKeyId));
}
}
- DiskEntry de = drs.getDiskEntry(key);
- if (de == null) {
- if (logger.isTraceEnabled(LogMarker.PERSIST_RECOVERY_VERBOSE)) {
- logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
- "readNewEntry oplogKeyId=<{}> drId={} userBits={} oplogOffset={} valueLen={}",
- oplogKeyId, drId, userBits, oplogOffset, valueLength);
- }
- DiskEntry.RecoveredEntry re = createRecoveredEntry(valueBytes, valueLength, userBits,
- getOplogId(), oplogOffset, oplogKeyId, false, version, in);
- if (tag != null) {
- re.setVersionTag(tag);
- }
- initRecoveredEntry(drs.getDiskRegionView(), drs.initializeRecoveredEntry(key, re));
- drs.getDiskRegionView().incRecoveredEntryCount();
- this.stats.incRecoveredEntryCreates();
- krfEntryCount++;
- } else {
- DiskId curdid = de.getDiskId();
- // assert curdid.getOplogId() != getOplogId();
- if (logger.isTraceEnabled(LogMarker.PERSIST_RECOVERY_VERBOSE)) {
- logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
- "ignore readNewEntry because getOplogId()={} != curdid.getOplogId()={} for drId={} key={}",
- getOplogId(), curdid.getOplogId(), drId, key);
+ if (drs != null) {
+ DiskEntry de = drs.getDiskEntry(key);
+ if (de == null) {
+ if (logger.isTraceEnabled(LogMarker.PERSIST_RECOVERY_VERBOSE)) {
+ logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
+ "readNewEntry oplogKeyId=<{}> drId={} userBits={} oplogOffset={} valueLen={}",
+ oplogKeyId, drId, userBits, oplogOffset, valueLength);
+ }
+ DiskEntry.RecoveredEntry re =
+ createRecoveredEntry(valueBytes, valueLength, userBits,
+ getOplogId(), oplogOffset, oplogKeyId, false, version, in);
+ if (tag != null) {
+ re.setVersionTag(tag);
+ }
+ initRecoveredEntry(drs.getDiskRegionView(), drs.initializeRecoveredEntry(key, re));
+ drs.getDiskRegionView().incRecoveredEntryCount();
+ this.stats.incRecoveredEntryCreates();
+ krfEntryCount++;
+ } else {
+ DiskId curdid = de.getDiskId();
+ // assert curdid.getOplogId() != getOplogId();
+ if (logger.isTraceEnabled(LogMarker.PERSIST_RECOVERY_VERBOSE)) {
+ logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
+ "ignore readNewEntry because getOplogId()={} != curdid.getOplogId()={} for drId={} key={}",
+ getOplogId(), curdid.getOplogId(), drId, key);
+ }
}
}
}
@@ -2541,10 +2544,12 @@ public class Oplog implements CompactableOplog, Flushable {
if (isPhase1()) {
CompactionRecord cr = new CompactionRecord(keyBytes, crOffset);
getRecoveryMap().put(oplogKeyId, cr);
- drs.getDiskRegionView().incRecoveredEntryCount();
+ if (drs != null) {
+ drs.getDiskRegionView().incRecoveredEntryCount();
+ }
this.stats.incRecoveredEntryCreates();
} else { // phase2
- Assert.assertTrue(p2cr != null, "First pass did not find create a compaction record");
+ Assert.assertNotNull(p2cr, "First pass did not find create a compaction record");
getOplogSet().getChild().copyForwardForOfflineCompact(oplogKeyId, p2cr.getKeyBytes(),
objValue, userBits, drId, tag);
if (isPersistRecoveryDebugEnabled) {
@@ -2566,29 +2571,31 @@ public class Oplog implements CompactableOplog, Flushable {
oplogKeyId));
}
}
- DiskEntry de = drs.getDiskEntry(key);
- if (de == null) {
- if (isPersistRecoveryDebugEnabled) {
- logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
- "readNewEntry oplogKeyId=<{}> drId={} key={} userBits={} oplogOffset={} valueLen={} tag={}",
- oplogKeyId, drId, key, userBits, oplogOffset, valueLength, tag);
- }
- DiskEntry.RecoveredEntry re = createRecoveredEntry(objValue, valueLength, userBits,
- getOplogId(), oplogOffset, oplogKeyId, recoverValue, version, in);
- if (tag != null) {
- re.setVersionTag(tag);
- }
- initRecoveredEntry(drs.getDiskRegionView(), drs.initializeRecoveredEntry(key, re));
- drs.getDiskRegionView().incRecoveredEntryCount();
- this.stats.incRecoveredEntryCreates();
+ if (drs != null) {
+ DiskEntry de = drs.getDiskEntry(key);
+ if (de == null) {
+ if (isPersistRecoveryDebugEnabled) {
+ logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
+ "readNewEntry oplogKeyId=<{}> drId={} key={} userBits={} oplogOffset={} valueLen={} tag={}",
+ oplogKeyId, drId, key, userBits, oplogOffset, valueLength, tag);
+ }
+ DiskEntry.RecoveredEntry re = createRecoveredEntry(objValue, valueLength, userBits,
+ getOplogId(), oplogOffset, oplogKeyId, recoverValue, version, in);
+ if (tag != null) {
+ re.setVersionTag(tag);
+ }
+ initRecoveredEntry(drs.getDiskRegionView(), drs.initializeRecoveredEntry(key, re));
+ drs.getDiskRegionView().incRecoveredEntryCount();
+ this.stats.incRecoveredEntryCreates();
- } else {
- DiskId curdid = de.getDiskId();
- assert curdid.getOplogId() != getOplogId();
- if (isPersistRecoveryDebugEnabled) {
- logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
- "ignore readNewEntry because getOplogId()={} != curdid.getOplogId()={} for drId={} key={}",
- getOplogId(), curdid.getOplogId(), drId, key);
+ } else {
+ DiskId curdid = de.getDiskId();
+ assert curdid.getOplogId() != getOplogId();
+ if (isPersistRecoveryDebugEnabled) {
+ logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
+ "ignore readNewEntry because getOplogId()={} != curdid.getOplogId()={} for drId={} key={}",
+ getOplogId(), curdid.getOplogId(), drId, key);
+ }
}
}
}
@@ -2719,7 +2726,7 @@ public class Oplog implements CompactableOplog, Flushable {
incSkipped(); // we are going to compact the previous record away
cr.update(crOffset);
} else { // phase2
- Assert.assertTrue(p2cr != null, "First pass did not find create a compaction record");
+ Assert.assertNotNull(p2cr, "First pass did not find create a compaction record");
getOplogSet().getChild().copyForwardForOfflineCompact(oplogKeyId, p2cr.getKeyBytes(),
objValue, userBits, drId, tag);
if (isPersistRecoveryDebugEnabled) {
@@ -2732,38 +2739,40 @@ public class Oplog implements CompactableOplog, Flushable {
} else {
// Check the actual region to see if it has this key from
// a previous recovered oplog.
- DiskEntry de = drs.getDiskEntry(key);
+ if (drs != null) {
+ DiskEntry de = drs.getDiskEntry(key);
- // This may actually be create, if the previous create or modify
- // of this entry was cleared through the RVV clear.
- if (de == null) {
- DiskRegionView drv = drs.getDiskRegionView();
- // and create an entry
- DiskEntry.RecoveredEntry re = createRecoveredEntry(objValue, valueLength, userBits,
- getOplogId(), oplogOffset, oplogKeyId, recoverValue, version, in);
- if (tag != null) {
- re.setVersionTag(tag);
- }
- if (isPersistRecoveryDebugEnabled) {
- logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
- "readModEntryWK init oplogKeyId=<{}> drId={} key=<{}> oplogOffset={} userBits={} valueLen={} tag={}",
- oplogKeyId, drId, key, oplogOffset, userBits, valueLength, tag);
- }
- initRecoveredEntry(drv, drs.initializeRecoveredEntry(key, re));
- drs.getDiskRegionView().incRecoveredEntryCount();
- this.stats.incRecoveredEntryCreates();
+ // This may actually be create, if the previous create or modify
+ // of this entry was cleared through the RVV clear.
+ if (de == null) {
+ DiskRegionView drv = drs.getDiskRegionView();
+ // and create an entry
+ DiskEntry.RecoveredEntry re = createRecoveredEntry(objValue, valueLength, userBits,
+ getOplogId(), oplogOffset, oplogKeyId, recoverValue, version, in);
+ if (tag != null) {
+ re.setVersionTag(tag);
+ }
+ if (isPersistRecoveryDebugEnabled) {
+ logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
+ "readModEntryWK init oplogKeyId=<{}> drId={} key=<{}> oplogOffset={} userBits={} valueLen={} tag={}",
+ oplogKeyId, drId, key, oplogOffset, userBits, valueLength, tag);
+ }
+ initRecoveredEntry(drv, drs.initializeRecoveredEntry(key, re));
+ drs.getDiskRegionView().incRecoveredEntryCount();
+ this.stats.incRecoveredEntryCreates();
- } else {
- DiskEntry.RecoveredEntry re = createRecoveredEntry(objValue, valueLength, userBits,
- getOplogId(), oplogOffset, oplogKeyId, recoverValue, version, in);
- if (tag != null) {
- re.setVersionTag(tag);
- }
- de = drs.updateRecoveredEntry(key, re);
- updateRecoveredEntry(drs.getDiskRegionView(), de, re);
+ } else {
+ DiskEntry.RecoveredEntry re = createRecoveredEntry(objValue, valueLength, userBits,
+ getOplogId(), oplogOffset, oplogKeyId, recoverValue, version, in);
+ if (tag != null) {
+ re.setVersionTag(tag);
+ }
+ de = drs.updateRecoveredEntry(key, re);
+ updateRecoveredEntry(drs.getDiskRegionView(), de, re);
- this.stats.incRecoveredEntryUpdates();
+ this.stats.incRecoveredEntryUpdates();
+ }
}
}
} else {
@@ -2936,10 +2945,12 @@ public class Oplog implements CompactableOplog, Flushable {
if (isPhase1()) {
CompactionRecord cr = new CompactionRecord(keyBytes, crOffset);
getRecoveryMap().put(oplogKeyId, cr);
- drs.getDiskRegionView().incRecoveredEntryCount();
+ if (drs != null) {
+ drs.getDiskRegionView().incRecoveredEntryCount();
+ }
this.stats.incRecoveredEntryCreates();
} else { // phase2
- Assert.assertTrue(p2cr != null, "First pass did not find create a compaction record");
+ Assert.assertNotNull(p2cr, "First pass did not find create a compaction record");
getOplogSet().getChild().copyForwardForOfflineCompact(oplogKeyId, p2cr.getKeyBytes(),
objValue, userBits, drId, tag);
if (logger.isTraceEnabled(LogMarker.PERSIST_RECOVERY_VERBOSE)) {
@@ -2960,34 +2971,37 @@ public class Oplog implements CompactableOplog, Flushable {
}
// Check the actual region to see if it has this key from
// a previous recovered oplog.
- DiskEntry de = drs.getDiskEntry(key);
- if (de == null) {
- DiskRegionView drv = drs.getDiskRegionView();
- // and create an entry
- DiskEntry.RecoveredEntry re = createRecoveredEntry(objValue, valueLength, userBits,
- getOplogId(), oplogOffset, oplogKeyId, recoverValue, version, in);
- if (tag != null) {
- re.setVersionTag(tag);
- }
- if (logger.isTraceEnabled(LogMarker.PERSIST_RECOVERY_VERBOSE)) {
- logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
- "readModEntryWK init oplogKeyId=<{}> drId={} key={} oplogOffset={} userBits={} valueLen={} tag={}",
- oplogKeyId, drId, key, oplogOffset, userBits, valueLength, tag);
- }
- initRecoveredEntry(drv, drs.initializeRecoveredEntry(key, re));
- drs.getDiskRegionView().incRecoveredEntryCount();
- this.stats.incRecoveredEntryCreates();
+ if (drs != null) {
+ DiskEntry de = drs.getDiskEntry(key);
+ if (de == null) {
+ DiskRegionView drv = drs.getDiskRegionView();
+ // and create an entry
+ DiskEntry.RecoveredEntry re = createRecoveredEntry(objValue, valueLength, userBits,
+ getOplogId(), oplogOffset, oplogKeyId, recoverValue, version, in);
+ if (tag != null) {
+ re.setVersionTag(tag);
+ }
+ if (logger.isTraceEnabled(LogMarker.PERSIST_RECOVERY_VERBOSE)) {
+ logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
+ "readModEntryWK init oplogKeyId=<{}> drId={} key={} oplogOffset={} userBits={} valueLen={} tag={}",
+ oplogKeyId, drId, key, oplogOffset, userBits, valueLength, tag);
+ }
+ initRecoveredEntry(drv, drs.initializeRecoveredEntry(key, re));
+ drs.getDiskRegionView().incRecoveredEntryCount();
+ this.stats.incRecoveredEntryCreates();
- } else {
- DiskId curdid = de.getDiskId();
- assert curdid
- .getOplogId() != getOplogId() : "Mutiple ModEntryWK in the same oplog for getOplogId()="
- + getOplogId() + " , curdid.getOplogId()=" + curdid.getOplogId() + " , for drId="
- + drId + " , key=" + key;
- if (logger.isTraceEnabled(LogMarker.PERSIST_RECOVERY_VERBOSE)) {
- logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
- "ignore readModEntryWK because getOplogId()={} != curdid.getOplogId()={} for drId={} key={}",
- getOplogId(), curdid.getOplogId(), drId, key);
+ } else {
+ DiskId curdid = de.getDiskId();
+ assert curdid
+ .getOplogId() != getOplogId() : "Mutiple ModEntryWK in the same oplog for getOplogId()="
+ + getOplogId() + " , curdid.getOplogId()=" + curdid.getOplogId()
+ + " , for drId="
+ + drId + " , key=" + key;
+ if (logger.isTraceEnabled(LogMarker.PERSIST_RECOVERY_VERBOSE)) {
+ logger.trace(LogMarker.PERSIST_RECOVERY_VERBOSE,
+ "ignore readModEntryWK because getOplogId()={} != curdid.getOplogId()={} for drId={} key={}",
+ getOplogId(), curdid.getOplogId(), drId, key);
+ }
}
}
}
@@ -3583,7 +3597,7 @@ public class Oplog implements CompactableOplog, Flushable {
logger.trace(LogMarker.PERSIST_WRITES_VERBOSE,
"basicCreate: id=<{}> key=<{}> valueOffset={} userBits={} valueLen={} valueBytes={} drId={} versionTag={} oplog#{}",
abs(id.getKeyId()), entry.getKey(), startPosForSynchOp, userBits,
- (value != null ? value.getLength() : 0), value.getBytesAsString(), dr.getId(), tag,
+ value.getLength(), value.getBytesAsString(), dr.getId(), tag,
getOplogId());
}
id.setOffsetInOplog(startPosForSynchOp);
@@ -5653,7 +5667,7 @@ public class Oplog implements CompactableOplog, Flushable {
if (!olf.f.exists())
return;
assert olf.RAFClosed;
- if (!olf.RAFClosed || olf.raf != null) {
+ if (olf.raf != null) {
try {
olf.raf.close();
olf.RAFClosed = true;
@@ -6521,7 +6535,7 @@ public class Oplog implements CompactableOplog, Flushable {
private final byte[] drIdBytes = new byte[DiskInitFile.DR_ID_MAX_BYTES];
private byte[] keyBytes;
private final byte[] deltaIdBytes = new byte[8];
- private byte deltaIdBytesLength;
+ private int deltaIdBytesLength;
private long newEntryBase;
private DiskStoreID diskStoreId;
private OPLOG_TYPE magic;
@@ -6653,7 +6667,7 @@ public class Oplog implements CompactableOplog, Flushable {
long delta = calcDelta(oplogKeyId, this.opCode);
this.deltaIdBytesLength = bytesNeeded(delta);
this.size += this.deltaIdBytesLength;
- this.opCode += (this.deltaIdBytesLength & 0xFF) - 1;
+ this.opCode += (byte) ((this.deltaIdBytesLength & 0xFF) - 1);
for (int i = this.deltaIdBytesLength - 1; i >= 0; i--) {
this.deltaIdBytes[i] = (byte) (delta & 0xFF);
delta >>= 8;
@@ -6750,7 +6764,7 @@ public class Oplog implements CompactableOplog, Flushable {
long delta = calcDelta(keyId, this.opCode);
this.deltaIdBytesLength = bytesNeeded(delta);
this.size += this.deltaIdBytesLength;
- this.opCode += (byte) (this.deltaIdBytesLength & 0xFF) - 1;
+ this.opCode += (byte) ((this.deltaIdBytesLength & 0xFF) - 1);
for (int i = this.deltaIdBytesLength - 1; i >= 0; i--) {
this.deltaIdBytes[i] = (byte) (delta & 0xFF);
delta >>= 8;
@@ -6848,11 +6862,12 @@ public class Oplog implements CompactableOplog, Flushable {
write(olf, this.drIdBytes, this.drIdLength);
bytesWritten += this.drIdLength;
}
- assert this.versionsBytes.length > 0;
- write(olf, this.versionsBytes, this.versionsBytes.length);
- bytesWritten += this.versionsBytes.length;
+ if (this.versionsBytes != null && this.versionsBytes.length > 0) {
+ write(olf, this.versionsBytes, this.versionsBytes.length);
+ bytesWritten += this.versionsBytes.length;
+ }
} else {
- if (this.notToUseUserBits == false) {
+ if (!this.notToUseUserBits) {
writeByte(olf, this.userBits);
bytesWritten++;
}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
index 0767ced..5ad77c6 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
@@ -911,9 +911,8 @@ public class PartitionedRegion extends LocalRegion
* Don't bother logging membership activity if our region isn't ready.
*/
if (isInitialized()) {
- CacheProfile cacheProfile =
- ((profile instanceof CacheProfile) ? (CacheProfile) profile : null);
- Set<String> onlineMembers = new HashSet<String>();
+ CacheProfile cacheProfile = (CacheProfile) profile;
+ Set<String> onlineMembers = new HashSet<>();
TransformUtils.transform(
PartitionedRegion.this.distAdvisor.advisePersistentMembers().values(), onlineMembers,
@@ -2514,8 +2513,8 @@ public class PartitionedRegion extends LocalRegion
}
this.prStats.endRemoveAll(startTime);
if (!keyToVersionMap.isEmpty()) {
- for (Iterator it = successfulOps.getKeys().iterator(); it.hasNext();) {
- successfulOps.addVersion(keyToVersionMap.get(it.next()));
+ for (Object o : successfulOps.getKeys()) {
+ successfulOps.addVersion(keyToVersionMap.get(o));
}
keyToVersionMap.clear();
}
@@ -2641,9 +2640,7 @@ public class PartitionedRegion extends LocalRegion
retryTime.waitToRetryNode();
}
event.setPossibleDuplicate(true);
- if (prMsg != null) {
- prMsg.setPossibleDuplicate(true);
- }
+ prMsg.setPossibleDuplicate(true);
} catch (PrimaryBucketException notPrimary) {
if (isDebugEnabled) {
logger.debug("Bucket {} on Node {} not primnary", notPrimary.getLocalizedMessage(),
@@ -7059,8 +7056,8 @@ public class PartitionedRegion extends LocalRegion
DLockService ls1 = lockService;
DLockService ls2 = other.lockService;
if (ls1 == null || ls2 == null) {
- if (ls1 != ls2)
- return false;
+ // If both are null, return true, if only one is null, return false
+ return ls1 == ls2;
}
return ls1.equals(ls2);
}
@@ -9056,20 +9053,21 @@ public class PartitionedRegion extends LocalRegion
* is the primary
* @throws ForceReattemptException if the caller should reattempt this request
*/
- public List getBucketOwnersForValidation(int bucketId) throws ForceReattemptException {
+ public List<Object[]> getBucketOwnersForValidation(int bucketId) throws ForceReattemptException {
// bucketid 1 => "vm A", false | "vm B", false | "vm C", true | "vm D", false
// bucketid 2 => List< Tuple(MemberId mem, Boolean isPrimary) >
// remotely fetch each VM's bucket meta-data (versus looking at the bucket
// advisor's data
RuntimeException rte = null;
- List remoteInfos = null;
+ List<Object[]> remoteInfos = new LinkedList<>();
for (int i = 0; i < 3; i++) {
rte = null;
DumpB2NResponse response =
DumpB2NRegion.send(getRegionAdvisor().adviseDataStore(), this, bucketId, true);
try {
- remoteInfos = new LinkedList(response.waitForPrimaryInfos());
+ remoteInfos.addAll(response.waitForPrimaryInfos());
+ break;
} catch (TimeoutException e) {
rte = e;
logger.info("DumpB2NRegion failed to get PR {}, bucket id {}'s info due to {}, retrying...",
@@ -9592,7 +9590,7 @@ public class PartitionedRegion extends LocalRegion
count++;
}
}
- Assert.assertTrue(br != null, "Could not create storage for Entry");
+ Assert.assertNotNull(br, "Could not create storage for Entry");
if (keyInfo.isCheckPrimary()) {
br.checkForPrimary();
}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/ha/HARegionQueue.java b/geode-core/src/main/java/org/apache/geode/internal/cache/ha/HARegionQueue.java
index eeb2b4a..c2f4a42 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/ha/HARegionQueue.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/ha/HARegionQueue.java
@@ -1824,7 +1824,7 @@ public class HARegionQueue implements RegionQueue {
InternalCqQuery cq =
((InternalCqQuery) cqService.getClientCqFromServer(clientProxyID, cqName));
CqQueryVsdStats cqStats = cq.getVsdStats();
- if (cq != null && cqStats != null) {
+ if (cqStats != null) {
cqStats.incNumHAQueuedEvents(incrementAmount);
}
}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/DumpB2NRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/DumpB2NRegion.java
index 52cdf06..a8e4fdf 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/DumpB2NRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/DumpB2NRegion.java
@@ -254,7 +254,7 @@ public class DumpB2NRegion extends PartitionMessage {
}
public static class DumpB2NResponse extends PartitionResponse {
- public final ArrayList primaryInfos = new ArrayList();
+ public final ArrayList<Object[]> primaryInfos = new ArrayList<>();
public DumpB2NResponse(InternalDistributedSystem dm, Set initMembers) {
super(dm, initMembers);
@@ -279,7 +279,7 @@ public class DumpB2NRegion extends PartitionMessage {
super.process(msg);
}
- public List waitForPrimaryInfos() throws ForceReattemptException {
+ public List<Object[]> waitForPrimaryInfos() throws ForceReattemptException {
try {
waitForCacheException();
} catch (ForceReattemptException e) {
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java b/geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
index 43fb4ce..1a18d8d 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
@@ -204,12 +204,11 @@ public class ParallelGatewaySenderQueue implements RegionQueue {
@Override
public void run() {
- PartitionedRegion prQ = null;
GatewaySenderEventImpl event = (GatewaySenderEventImpl) conflatableObject;
+ String regionPath =
+ ColocationHelper.getLeaderRegion((PartitionedRegion) event.getRegion()).getFullPath();
+ PartitionedRegion prQ = userRegionNameToShadowPRMap.get(regionPath);
try {
- String regionPath =
- ColocationHelper.getLeaderRegion((PartitionedRegion) event.getRegion()).getFullPath();
- prQ = userRegionNameToShadowPRMap.get(regionPath);
destroyEventFromQueue(prQ, bucketId, previousTailKeyTobeRemoved);
} catch (EntryNotFoundException e) {
if (logger.isDebugEnabled()) {
@@ -232,7 +231,7 @@ public class ParallelGatewaySenderQueue implements RegionQueue {
// ClassNotFoundException
try {
deserializedObject = EntryEventImpl.deserialize(serializedBytesCast);
- } catch (Exception e) {
+ } catch (Exception ignore) {
}
}
return deserializedObject;
@@ -1583,7 +1582,7 @@ public class ParallelGatewaySenderQueue implements RegionQueue {
public int localSize(boolean includeSecondary) {
int size = 0;
for (PartitionedRegion prQ : this.userRegionNameToShadowPRMap.values()) {
- if (prQ != null && prQ.getDataStore() != null) {
+ if (prQ.getDataStore() != null) {
if (includeSecondary) {
size += prQ.getDataStore().getSizeOfLocalBuckets();
} else {
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/security/MBeanServerWrapper.java b/geode-core/src/main/java/org/apache/geode/management/internal/security/MBeanServerWrapper.java
index eb31a45..4b1a575 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/security/MBeanServerWrapper.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/security/MBeanServerWrapper.java
@@ -15,6 +15,7 @@
package org.apache.geode.management.internal.security;
import java.io.ObjectInputStream;
+import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
@@ -182,7 +183,7 @@ public class MBeanServerWrapper implements MBeanServerForwarder {
results = mbs.getAttributes(name, attributes);
} catch (Exception e) {
throw new GemFireSecurityException(
- "error getting values of attributes :" + attributes + " from " + name,
+ "error getting values of attributes :" + Arrays.toString(attributes) + " from " + name,
e);
}
return results;
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java
index 67f9355..69f66aa 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java
@@ -115,10 +115,12 @@ public class AbstractRegionJUnitTest {
DiskWriteAttributes diskWriteAttributes = mock(DiskWriteAttributes.class);
when(regionAttributes.getDiskWriteAttributes()).thenReturn(diskWriteAttributes);
RegionMapConstructor regionMapConstructor = mock(RegionMapConstructor.class);
+ LocalRegion.ServerRegionProxyConstructor proxyConstructor = mock(
+ LocalRegion.ServerRegionProxyConstructor.class);
Function<LocalRegion, RegionPerfStats> regionPerfStatsFactory =
(localRegion) -> mock(RegionPerfStats.class);
AbstractRegion region = new LocalRegion("regionName", regionAttributes, null, Fakes.cache(),
- new InternalRegionArguments(), null, regionMapConstructor, null, null, null,
+ new InternalRegionArguments(), null, regionMapConstructor, proxyConstructor, null, null,
regionPerfStatsFactory, disabledClock());
return region;
}
diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
index b4c77c2..1d7611a 100644
--- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
+++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
@@ -1716,7 +1716,8 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
if (!foundRemoteId) {
try {
- if (currentLatch.await(membershipCheckTimeout, TimeUnit.MILLISECONDS)) {
+ if (currentLatch != null
+ && currentLatch.await(membershipCheckTimeout, TimeUnit.MILLISECONDS)) {
foundRemoteId = true;
// @todo
// ARB: remove latch from memberLatch map if this is the last thread waiting on latch.