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/05/19 05:47:06 UTC
[GitHub] [geode] pivotal-jbarrett opened a new pull request #6500: GEODE-8870: Removes GFE_70.
pivotal-jbarrett opened a new pull request #6500:
URL: https://github.com/apache/geode/pull/6500
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #6500:
URL: https://github.com/apache/geode/pull/6500#issuecomment-868779630
> Sure but GEODE-6588 could have been another PRs and limit this PR to GEODE-8870
Is the issue that there is cleanup of files touched by the fix in this PR as separate commits or that those commits have a different JIRA issue associated with them?
--
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 #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r636252877
##########
File path: geode-core/src/test/java/org/apache/geode/internal/DataSerializableJUnitTest.java
##########
@@ -1166,13 +1166,13 @@ public void testVersionedDataSerializable() throws Exception {
VersionedDataSerializableImpl ds = new VersionedDataSerializableImpl(getRandom());
VersionedDataOutputStream v =
- new VersionedDataOutputStream(this.baos, KnownVersion.GFE_70);
+ new VersionedDataOutputStream(this.baos, KnownVersion.GEODE_1_11_0);
Review comment:
Is there a specific reason for this version to be used here? Similarly for the use of `GEODE_1_12_0` below.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -5723,89 +5590,79 @@ public DirectoryHolder getDirectoryHolder() {
* @return long value indicating the current size of the oplog.
*/
long getOplogSize() {
- return this.crf.currSize + this.drf.currSize;
+ return crf.currSize + drf.currSize;
}
boolean isOplogEmpty() {
- return this.crf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE)
- && this.drf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE);
+ return crf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE)
+ && drf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE);
}
void incLiveCount() {
- this.totalLiveCount.incrementAndGet();
+ totalLiveCount.incrementAndGet();
}
private void decLiveCount() {
- this.totalLiveCount.decrementAndGet();
+ totalLiveCount.decrementAndGet();
}
/**
* Return true if a record (crf or drf) has been added to this oplog
*/
boolean hasBeenUsed() {
- return this.hasDeletes.get() || this.totalCount.get() > 0;
+ return hasDeletes.get() || totalCount.get() > 0;
}
void incTotalCount() {
if (!isPhase2()) {
- this.totalCount.incrementAndGet();
+ totalCount.incrementAndGet();
}
}
private void finishedAppending() {
// No need to get the backup lock prior to synchronizing (correct lock order) since the
// synchronized block does not attempt to get the backup lock (incorrect lock order)
- synchronized (this.lock/* crf */) {
- this.doneAppending = true;
+ synchronized (lock/* crf */) {
+ doneAppending = true;
}
handleNoLiveValues();
// I'm deadcoding the following because it is not safe unless we change to
// always recover values. If we don't recover values then
// an oplog we recovered from may still need to fault values in from memory.
Review comment:
This comment no longer refers to any code, commented out or otherwise, so it should also be removed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout commented on a change in pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r636913061
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/persistence/DiskInitFileParser.java
##########
@@ -567,24 +568,20 @@ private void readEndOfRecord(DataInput di) throws IOException {
}
}
- private PersistentMemberID readPMID(CountingDataInputStream dis, KnownVersion gfversion)
+ private PersistentMemberID readPMID(CountingDataInputStream dis)
throws IOException, ClassNotFoundException {
int len = dis.readInt();
byte[] buf = new byte[len];
dis.readFully(buf);
- return bytesToPMID(buf, gfversion);
+ return bytesToPMID(buf);
}
- private PersistentMemberID bytesToPMID(byte[] bytes, KnownVersion gfversion)
+ private PersistentMemberID bytesToPMID(byte[] bytes)
throws IOException, ClassNotFoundException {
ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
DataInputStream dis = new DataInputStream(bais);
PersistentMemberID result = new PersistentMemberID();
- if (KnownVersion.GFE_70.compareTo(gfversion) > 0) {
- result._fromData662(dis);
- } else {
- InternalDataSerializer.invokeFromData(result, dis);
- }
+ InternalDataSerializer.invokeFromData(result, dis);
Review comment:
In general, when we read from disk or the network we should be creating a VersionedDataInputStream. That allows us to make DataSerializableFixedId objects backwards compatible just by adding special fromData_Pre_XXX methods.
Maybe we should create one of those here, using gfVersion.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #6500:
URL: https://github.com/apache/geode/pull/6500#issuecomment-868747015
> Also, I would request refactorings to be kept in separate PRs.
>
> * The caption JIRA mentions that this GEODE-8870 ticket is for removal of obsolete GFE_70 code but has GEODE-6588 along with it which has other major refactorings.
> * Keeping the CL shorter will have faster approval / thorough review as the reviewer list smaller
> * We view the final set of files changed as it is more convenient than looking at each and every commit individually, as the same file may have been modified in multiple commits.
I think you should take this preference to a discussion on dev@geode. We have had similar discussions without an clear consensus. The closest we got was around not forcing squashes on PRs that include explicitly distinguished fix and cleanup commits, like this one.
https://markmail.org/message/hflkinvcxjg6stbo
--
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 pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #6500:
URL: https://github.com/apache/geode/pull/6500#issuecomment-854149682
> I see lot more refactoring changes than the fix for actual issue. I agree and believe in incremental refactoring as we come across the opportunity; but here I see lot more refactoring changes; even in non-related files (where the actual change is not expected); it will be nice to keep those changes minimal and focused on the issue being fixed...just my thoughts and I am fine with other way too...
The fix and the cleanup on on two separate commits to make reviewing easier. The other cleaned files are files affected by the correction of issues in a dependent 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on pull request #6500:
URL: https://github.com/apache/geode/pull/6500#issuecomment-868749183
Sure but GEODE-6588 could have been another PRs and limit this PR to GEODE-8870
--
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 #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r658946027
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/TXCommitMessage.java
##########
@@ -1384,153 +1323,145 @@ protected void txApplyEntryOpAdjunctOnly(FarSideEntryOp entryOp) {
*/
@Released
EntryEventImpl eei =
- txCallbackEventFactory.createCallbackEvent(this.internalRegion, entryOp.op,
+ txCallbackEventFactory.createCallbackEvent(internalRegion, entryOp.op,
entryOp.key,
- entryOp.value, this.msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
- entryOp.filterRoutingInfo, this.msg.bridgeContext, null, entryOp.versionTag,
+ entryOp.value, msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
+ entryOp.filterRoutingInfo, msg.bridgeContext, null, entryOp.versionTag,
entryOp.tailKey);
try {
if (entryOp.filterRoutingInfo != null) {
eei.setLocalFilterInfo(
- entryOp.filterRoutingInfo.getFilterInfo(this.internalRegion.getCache().getMyId()));
+ entryOp.filterRoutingInfo.getFilterInfo(internalRegion.getCache().getMyId()));
}
if (isDuplicate) {
eei.setPossibleDuplicate(true);
}
if (logger.isDebugEnabled()) {
logger.debug("invoking transactional callbacks for {} key={} needsUnlock={} event={}",
- entryOp.op, entryOp.key, this.needsUnlock, eei);
+ entryOp.op, entryOp.key, needsUnlock, eei);
}
// we reach this spot because the event is either delivered to this member
// as an "adjunct" message or because the bucket was being created when
// the message was sent and already reflects the change caused by this event.
// In the latter case we need to invoke listeners
final boolean skipListeners = !isDuplicate;
- eei.invokeCallbacks(this.internalRegion, skipListeners, true);
+ eei.invokeCallbacks(internalRegion, skipListeners, true);
} finally {
eei.release();
}
- return;
}
}
boolean isEmpty() {
- return this.opKeys == null;
+ return opKeys == null;
}
boolean needsAck() {
- return this.internalRegion.getScope().isDistributedAck();
+ return internalRegion.getScope().isDistributedAck();
}
void addOp(Object key, TXEntryState entry) {
- if (this.opKeys == null) {
- this.opKeys = new ArrayList(this.maxSize);
- this.opEntries = new ArrayList(this.maxSize);
+ if (opKeys == null) {
+ opKeys = new ArrayList<>(maxSize);
+ opEntries = new ArrayList<>(maxSize);
}
- this.opKeys.add(key);
- this.opEntries.add(entry);
+ opKeys.add(key);
+ opEntries.add(entry);
}
public boolean isForceFireEvent(DistributionManager dm) {
LocalRegion r = getRegionByPath(dm, regionPath);
- if (r instanceof PartitionedRegion || (r != null && r.isUsedForPartitionedRegionBucket())) {
- return false;
- }
- return true;
+ return !(r instanceof PartitionedRegion) && (r == null || !r
+ .isUsedForPartitionedRegionBucket());
}
public void fromData(DataInput in, boolean hasShadowKey)
throws IOException, ClassNotFoundException {
- this.regionPath = DataSerializer.readString(in);
- this.parentRegionPath = DataSerializer.readString(in);
+ regionPath = DataSerializer.readString(in);
+ parentRegionPath = DataSerializer.readString(in);
int size = in.readInt();
if (size > 0) {
- this.opKeys = new ArrayList(size);
- this.opEntries = new ArrayList(size);
+ opKeys = new ArrayList<>(size);
+ opEntries = new ArrayList<>(size);
final boolean largeModCount = in.readBoolean();
- this.memberId = DataSerializer.readObject(in);
+ memberId = DataSerializer.readObject(in);
for (int i = 0; i < size; i++) {
FarSideEntryOp entryOp = new FarSideEntryOp();
// shadowkey is not being sent to clients
entryOp.fromData(in, largeModCount, hasShadowKey);
- if (entryOp.versionTag != null && this.memberId != null) {
- entryOp.versionTag.setMemberID(this.memberId);
+ if (entryOp.versionTag != null && memberId != null) {
+ entryOp.versionTag.setMemberID(memberId);
}
- this.msg.addFarSideEntryOp(entryOp);
- this.opKeys.add(entryOp.key);
- this.opEntries.add(entryOp);
+ msg.addFarSideEntryOp(entryOp);
+ opKeys.add(entryOp.key);
+ opEntries.add(entryOp);
}
}
}
@Override
public String toString() {
StringBuilder result = new StringBuilder(64);
- if (this.regionPath != null) {
- result.append(this.regionPath);
+ if (regionPath != null) {
+ result.append(regionPath);
} else {
- result.append(this.internalRegion.getFullPath());
+ result.append(internalRegion.getFullPath());
}
- if (this.refCount > 0) {
- result.append(" refCount=").append(this.refCount);
+ if (refCount > 0) {
+ result.append(" refCount=").append(refCount);
}
return result.toString();
}
private void basicToData(DataOutput out,
SerializationContext context,
boolean useShadowKey) throws IOException {
- if (this.internalRegion != null) {
- DataSerializer.writeString(this.internalRegion.getFullPath(), out);
- if (this.internalRegion instanceof BucketRegion) {
+ if (internalRegion != null) {
+ DataSerializer.writeString(internalRegion.getFullPath(), out);
+ if (internalRegion instanceof BucketRegion) {
DataSerializer.writeString(
- ((Bucket) this.internalRegion).getPartitionedRegion().getFullPath(), out);
+ ((Bucket) internalRegion).getPartitionedRegion().getFullPath(), out);
} else {
DataSerializer.writeString(null, out);
}
} else {
- DataSerializer.writeString(this.regionPath, out);
- DataSerializer.writeString(this.parentRegionPath, out);
+ DataSerializer.writeString(regionPath, out);
+ DataSerializer.writeString(parentRegionPath, out);
}
- if (isEmpty() || this.opKeys.size() == 0) {
+ if (isEmpty() || opKeys.size() == 0) {
out.writeInt(0);
} else {
- int size = this.opKeys.size();
+ int size = opKeys.size();
out.writeInt(size);
final boolean largeModCount;
- if (this.msg.txState != null) {
- largeModCount = this.msg.txState.needsLargeModCount();
+ if (msg.txState != null) {
+ largeModCount = msg.txState.needsLargeModCount();
} else {
- largeModCount = this.msg.needsLargeModCount;
+ largeModCount = msg.needsLargeModCount;
}
out.writeBoolean(largeModCount);
- final boolean sendVersionTags =
- this.msg.clientVersion == null
- || KnownVersion.GFE_70.compareTo(this.msg.clientVersion) <= 0;
- if (sendVersionTags) {
- VersionSource member = this.memberId;
- if (member == null) {
- if (this.internalRegion == null) {
- Assert.assertTrue(this.msg.txState == null);
- } else {
- member = this.internalRegion.getVersionMember();
- }
+ VersionSource member = memberId;
+ if (member == null) {
+ if (internalRegion == null) {
+ Assert.assertTrue(msg.txState == null);
+ } else {
+ member = internalRegion.getVersionMember();
}
- DataSerializer.writeObject(member, out);
}
+ DataSerializer.writeObject(member, out);
for (int i = 0; i < size; i++) {
- DataSerializer.writeObject(this.opKeys.get(i), out);
- if (this.msg.txState != null) {
+ DataSerializer.writeObject(opKeys.get(i), out);
+ if (msg.txState != null) {
/* we are still on tx node and have the entry state */
- ((TXEntryState) this.opEntries.get(i)).toFarSideData(out, context, largeModCount,
- sendVersionTags, useShadowKey);
+ ((TXEntryState) opEntries.get(i)).toFarSideData(out, context, largeModCount,
+ true, useShadowKey);
Review comment:
For the same reason as above.
--
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 #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r636300684
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -5723,89 +5590,79 @@ public DirectoryHolder getDirectoryHolder() {
* @return long value indicating the current size of the oplog.
*/
long getOplogSize() {
- return this.crf.currSize + this.drf.currSize;
+ return crf.currSize + drf.currSize;
}
boolean isOplogEmpty() {
- return this.crf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE)
- && this.drf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE);
+ return crf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE)
+ && drf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE);
}
void incLiveCount() {
- this.totalLiveCount.incrementAndGet();
+ totalLiveCount.incrementAndGet();
}
private void decLiveCount() {
- this.totalLiveCount.decrementAndGet();
+ totalLiveCount.decrementAndGet();
}
/**
* Return true if a record (crf or drf) has been added to this oplog
*/
boolean hasBeenUsed() {
- return this.hasDeletes.get() || this.totalCount.get() > 0;
+ return hasDeletes.get() || totalCount.get() > 0;
}
void incTotalCount() {
if (!isPhase2()) {
- this.totalCount.incrementAndGet();
+ totalCount.incrementAndGet();
}
}
private void finishedAppending() {
// No need to get the backup lock prior to synchronizing (correct lock order) since the
// synchronized block does not attempt to get the backup lock (incorrect lock order)
- synchronized (this.lock/* crf */) {
- this.doneAppending = true;
+ synchronized (lock/* crf */) {
+ doneAppending = true;
}
handleNoLiveValues();
// I'm deadcoding the following because it is not safe unless we change to
// always recover values. If we don't recover values then
// an oplog we recovered from may still need to fault values in from memory.
Review comment:
I removed this comment in a pending commit. I don't want to push it until other's have reviewed since it will kick off a full PR run without any code changes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a change in pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r658939295
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/TXCommitMessage.java
##########
@@ -1384,153 +1323,145 @@ protected void txApplyEntryOpAdjunctOnly(FarSideEntryOp entryOp) {
*/
@Released
EntryEventImpl eei =
- txCallbackEventFactory.createCallbackEvent(this.internalRegion, entryOp.op,
+ txCallbackEventFactory.createCallbackEvent(internalRegion, entryOp.op,
entryOp.key,
- entryOp.value, this.msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
- entryOp.filterRoutingInfo, this.msg.bridgeContext, null, entryOp.versionTag,
+ entryOp.value, msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
+ entryOp.filterRoutingInfo, msg.bridgeContext, null, entryOp.versionTag,
entryOp.tailKey);
try {
if (entryOp.filterRoutingInfo != null) {
eei.setLocalFilterInfo(
- entryOp.filterRoutingInfo.getFilterInfo(this.internalRegion.getCache().getMyId()));
+ entryOp.filterRoutingInfo.getFilterInfo(internalRegion.getCache().getMyId()));
}
if (isDuplicate) {
eei.setPossibleDuplicate(true);
}
if (logger.isDebugEnabled()) {
logger.debug("invoking transactional callbacks for {} key={} needsUnlock={} event={}",
- entryOp.op, entryOp.key, this.needsUnlock, eei);
+ entryOp.op, entryOp.key, needsUnlock, eei);
}
// we reach this spot because the event is either delivered to this member
// as an "adjunct" message or because the bucket was being created when
// the message was sent and already reflects the change caused by this event.
// In the latter case we need to invoke listeners
final boolean skipListeners = !isDuplicate;
- eei.invokeCallbacks(this.internalRegion, skipListeners, true);
+ eei.invokeCallbacks(internalRegion, skipListeners, true);
} finally {
eei.release();
}
- return;
}
}
boolean isEmpty() {
- return this.opKeys == null;
+ return opKeys == null;
}
boolean needsAck() {
- return this.internalRegion.getScope().isDistributedAck();
+ return internalRegion.getScope().isDistributedAck();
}
void addOp(Object key, TXEntryState entry) {
- if (this.opKeys == null) {
- this.opKeys = new ArrayList(this.maxSize);
- this.opEntries = new ArrayList(this.maxSize);
+ if (opKeys == null) {
+ opKeys = new ArrayList<>(maxSize);
+ opEntries = new ArrayList<>(maxSize);
}
- this.opKeys.add(key);
- this.opEntries.add(entry);
+ opKeys.add(key);
+ opEntries.add(entry);
}
public boolean isForceFireEvent(DistributionManager dm) {
LocalRegion r = getRegionByPath(dm, regionPath);
- if (r instanceof PartitionedRegion || (r != null && r.isUsedForPartitionedRegionBucket())) {
- return false;
- }
- return true;
+ return !(r instanceof PartitionedRegion) && (r == null || !r
+ .isUsedForPartitionedRegionBucket());
}
public void fromData(DataInput in, boolean hasShadowKey)
throws IOException, ClassNotFoundException {
- this.regionPath = DataSerializer.readString(in);
- this.parentRegionPath = DataSerializer.readString(in);
+ regionPath = DataSerializer.readString(in);
+ parentRegionPath = DataSerializer.readString(in);
int size = in.readInt();
if (size > 0) {
- this.opKeys = new ArrayList(size);
- this.opEntries = new ArrayList(size);
+ opKeys = new ArrayList<>(size);
+ opEntries = new ArrayList<>(size);
final boolean largeModCount = in.readBoolean();
- this.memberId = DataSerializer.readObject(in);
+ memberId = DataSerializer.readObject(in);
for (int i = 0; i < size; i++) {
FarSideEntryOp entryOp = new FarSideEntryOp();
// shadowkey is not being sent to clients
entryOp.fromData(in, largeModCount, hasShadowKey);
- if (entryOp.versionTag != null && this.memberId != null) {
- entryOp.versionTag.setMemberID(this.memberId);
+ if (entryOp.versionTag != null && memberId != null) {
+ entryOp.versionTag.setMemberID(memberId);
}
- this.msg.addFarSideEntryOp(entryOp);
- this.opKeys.add(entryOp.key);
- this.opEntries.add(entryOp);
+ msg.addFarSideEntryOp(entryOp);
+ opKeys.add(entryOp.key);
+ opEntries.add(entryOp);
}
}
}
@Override
public String toString() {
StringBuilder result = new StringBuilder(64);
- if (this.regionPath != null) {
- result.append(this.regionPath);
+ if (regionPath != null) {
+ result.append(regionPath);
} else {
- result.append(this.internalRegion.getFullPath());
+ result.append(internalRegion.getFullPath());
}
- if (this.refCount > 0) {
- result.append(" refCount=").append(this.refCount);
+ if (refCount > 0) {
+ result.append(" refCount=").append(refCount);
}
return result.toString();
}
private void basicToData(DataOutput out,
SerializationContext context,
boolean useShadowKey) throws IOException {
- if (this.internalRegion != null) {
- DataSerializer.writeString(this.internalRegion.getFullPath(), out);
- if (this.internalRegion instanceof BucketRegion) {
+ if (internalRegion != null) {
+ DataSerializer.writeString(internalRegion.getFullPath(), out);
+ if (internalRegion instanceof BucketRegion) {
DataSerializer.writeString(
- ((Bucket) this.internalRegion).getPartitionedRegion().getFullPath(), out);
+ ((Bucket) internalRegion).getPartitionedRegion().getFullPath(), out);
} else {
DataSerializer.writeString(null, out);
}
} else {
- DataSerializer.writeString(this.regionPath, out);
- DataSerializer.writeString(this.parentRegionPath, out);
+ DataSerializer.writeString(regionPath, out);
+ DataSerializer.writeString(parentRegionPath, out);
}
- if (isEmpty() || this.opKeys.size() == 0) {
+ if (isEmpty() || opKeys.size() == 0) {
out.writeInt(0);
} else {
- int size = this.opKeys.size();
+ int size = opKeys.size();
out.writeInt(size);
final boolean largeModCount;
- if (this.msg.txState != null) {
- largeModCount = this.msg.txState.needsLargeModCount();
+ if (msg.txState != null) {
+ largeModCount = msg.txState.needsLargeModCount();
} else {
- largeModCount = this.msg.needsLargeModCount;
+ largeModCount = msg.needsLargeModCount;
}
out.writeBoolean(largeModCount);
- final boolean sendVersionTags =
- this.msg.clientVersion == null
- || KnownVersion.GFE_70.compareTo(this.msg.clientVersion) <= 0;
- if (sendVersionTags) {
- VersionSource member = this.memberId;
- if (member == null) {
- if (this.internalRegion == null) {
- Assert.assertTrue(this.msg.txState == null);
- } else {
- member = this.internalRegion.getVersionMember();
- }
+ VersionSource member = memberId;
+ if (member == null) {
+ if (internalRegion == null) {
+ Assert.assertTrue(msg.txState == null);
+ } else {
+ member = internalRegion.getVersionMember();
}
- DataSerializer.writeObject(member, out);
}
+ DataSerializer.writeObject(member, out);
for (int i = 0; i < size; i++) {
- DataSerializer.writeObject(this.opKeys.get(i), out);
- if (this.msg.txState != null) {
+ DataSerializer.writeObject(opKeys.get(i), out);
+ if (msg.txState != null) {
/* we are still on tx node and have the entry state */
- ((TXEntryState) this.opEntries.get(i)).toFarSideData(out, context, largeModCount,
- sendVersionTags, useShadowKey);
+ ((TXEntryState) opEntries.get(i)).toFarSideData(out, context, largeModCount,
+ true, useShadowKey);
Review comment:
Now sendVersionTags is always set to true irrespective of if msg.clientVersion is null or not.
--
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 pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #6500:
URL: https://github.com/apache/geode/pull/6500#issuecomment-854149682
> I see lot more refactoring changes than the fix for actual issue. I agree and believe in incremental refactoring as we come across the opportunity; but here I see lot more refactoring changes; even in non-related files (where the actual change is not expected); it will be nice to keep those changes minimal and focused on the issue being fixed...just my thoughts and I am fine with other way too...
The fix and the cleanup on on two separate commits to make reviewing easier. The other cleaned files are files affected by the correction of issues in a dependent 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett merged pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett merged pull request #6500:
URL: https://github.com/apache/geode/pull/6500
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a change in pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r658951038
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/TXCommitMessage.java
##########
@@ -1384,153 +1323,145 @@ protected void txApplyEntryOpAdjunctOnly(FarSideEntryOp entryOp) {
*/
@Released
EntryEventImpl eei =
- txCallbackEventFactory.createCallbackEvent(this.internalRegion, entryOp.op,
+ txCallbackEventFactory.createCallbackEvent(internalRegion, entryOp.op,
entryOp.key,
- entryOp.value, this.msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
- entryOp.filterRoutingInfo, this.msg.bridgeContext, null, entryOp.versionTag,
+ entryOp.value, msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
+ entryOp.filterRoutingInfo, msg.bridgeContext, null, entryOp.versionTag,
entryOp.tailKey);
try {
if (entryOp.filterRoutingInfo != null) {
eei.setLocalFilterInfo(
- entryOp.filterRoutingInfo.getFilterInfo(this.internalRegion.getCache().getMyId()));
+ entryOp.filterRoutingInfo.getFilterInfo(internalRegion.getCache().getMyId()));
}
if (isDuplicate) {
eei.setPossibleDuplicate(true);
}
if (logger.isDebugEnabled()) {
logger.debug("invoking transactional callbacks for {} key={} needsUnlock={} event={}",
- entryOp.op, entryOp.key, this.needsUnlock, eei);
+ entryOp.op, entryOp.key, needsUnlock, eei);
}
// we reach this spot because the event is either delivered to this member
// as an "adjunct" message or because the bucket was being created when
// the message was sent and already reflects the change caused by this event.
// In the latter case we need to invoke listeners
final boolean skipListeners = !isDuplicate;
- eei.invokeCallbacks(this.internalRegion, skipListeners, true);
+ eei.invokeCallbacks(internalRegion, skipListeners, true);
} finally {
eei.release();
}
- return;
}
}
boolean isEmpty() {
- return this.opKeys == null;
+ return opKeys == null;
}
boolean needsAck() {
- return this.internalRegion.getScope().isDistributedAck();
+ return internalRegion.getScope().isDistributedAck();
}
void addOp(Object key, TXEntryState entry) {
- if (this.opKeys == null) {
- this.opKeys = new ArrayList(this.maxSize);
- this.opEntries = new ArrayList(this.maxSize);
+ if (opKeys == null) {
+ opKeys = new ArrayList<>(maxSize);
+ opEntries = new ArrayList<>(maxSize);
}
- this.opKeys.add(key);
- this.opEntries.add(entry);
+ opKeys.add(key);
+ opEntries.add(entry);
}
public boolean isForceFireEvent(DistributionManager dm) {
LocalRegion r = getRegionByPath(dm, regionPath);
- if (r instanceof PartitionedRegion || (r != null && r.isUsedForPartitionedRegionBucket())) {
- return false;
- }
- return true;
+ return !(r instanceof PartitionedRegion) && (r == null || !r
+ .isUsedForPartitionedRegionBucket());
}
public void fromData(DataInput in, boolean hasShadowKey)
throws IOException, ClassNotFoundException {
- this.regionPath = DataSerializer.readString(in);
- this.parentRegionPath = DataSerializer.readString(in);
+ regionPath = DataSerializer.readString(in);
+ parentRegionPath = DataSerializer.readString(in);
int size = in.readInt();
if (size > 0) {
- this.opKeys = new ArrayList(size);
- this.opEntries = new ArrayList(size);
+ opKeys = new ArrayList<>(size);
+ opEntries = new ArrayList<>(size);
final boolean largeModCount = in.readBoolean();
- this.memberId = DataSerializer.readObject(in);
+ memberId = DataSerializer.readObject(in);
for (int i = 0; i < size; i++) {
FarSideEntryOp entryOp = new FarSideEntryOp();
// shadowkey is not being sent to clients
entryOp.fromData(in, largeModCount, hasShadowKey);
- if (entryOp.versionTag != null && this.memberId != null) {
- entryOp.versionTag.setMemberID(this.memberId);
+ if (entryOp.versionTag != null && memberId != null) {
+ entryOp.versionTag.setMemberID(memberId);
}
- this.msg.addFarSideEntryOp(entryOp);
- this.opKeys.add(entryOp.key);
- this.opEntries.add(entryOp);
+ msg.addFarSideEntryOp(entryOp);
+ opKeys.add(entryOp.key);
+ opEntries.add(entryOp);
}
}
}
@Override
public String toString() {
StringBuilder result = new StringBuilder(64);
- if (this.regionPath != null) {
- result.append(this.regionPath);
+ if (regionPath != null) {
+ result.append(regionPath);
} else {
- result.append(this.internalRegion.getFullPath());
+ result.append(internalRegion.getFullPath());
}
- if (this.refCount > 0) {
- result.append(" refCount=").append(this.refCount);
+ if (refCount > 0) {
+ result.append(" refCount=").append(refCount);
}
return result.toString();
}
private void basicToData(DataOutput out,
SerializationContext context,
boolean useShadowKey) throws IOException {
- if (this.internalRegion != null) {
- DataSerializer.writeString(this.internalRegion.getFullPath(), out);
- if (this.internalRegion instanceof BucketRegion) {
+ if (internalRegion != null) {
+ DataSerializer.writeString(internalRegion.getFullPath(), out);
+ if (internalRegion instanceof BucketRegion) {
DataSerializer.writeString(
- ((Bucket) this.internalRegion).getPartitionedRegion().getFullPath(), out);
+ ((Bucket) internalRegion).getPartitionedRegion().getFullPath(), out);
} else {
DataSerializer.writeString(null, out);
}
} else {
- DataSerializer.writeString(this.regionPath, out);
- DataSerializer.writeString(this.parentRegionPath, out);
+ DataSerializer.writeString(regionPath, out);
+ DataSerializer.writeString(parentRegionPath, out);
}
- if (isEmpty() || this.opKeys.size() == 0) {
+ if (isEmpty() || opKeys.size() == 0) {
out.writeInt(0);
} else {
- int size = this.opKeys.size();
+ int size = opKeys.size();
out.writeInt(size);
final boolean largeModCount;
- if (this.msg.txState != null) {
- largeModCount = this.msg.txState.needsLargeModCount();
+ if (msg.txState != null) {
+ largeModCount = msg.txState.needsLargeModCount();
} else {
- largeModCount = this.msg.needsLargeModCount;
+ largeModCount = msg.needsLargeModCount;
}
out.writeBoolean(largeModCount);
- final boolean sendVersionTags =
- this.msg.clientVersion == null
- || KnownVersion.GFE_70.compareTo(this.msg.clientVersion) <= 0;
- if (sendVersionTags) {
- VersionSource member = this.memberId;
- if (member == null) {
- if (this.internalRegion == null) {
- Assert.assertTrue(this.msg.txState == null);
- } else {
- member = this.internalRegion.getVersionMember();
- }
+ VersionSource member = memberId;
Review comment:
Thank you for the explanation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a change in pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r658938625
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/TXCommitMessage.java
##########
@@ -1384,153 +1323,145 @@ protected void txApplyEntryOpAdjunctOnly(FarSideEntryOp entryOp) {
*/
@Released
EntryEventImpl eei =
- txCallbackEventFactory.createCallbackEvent(this.internalRegion, entryOp.op,
+ txCallbackEventFactory.createCallbackEvent(internalRegion, entryOp.op,
entryOp.key,
- entryOp.value, this.msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
- entryOp.filterRoutingInfo, this.msg.bridgeContext, null, entryOp.versionTag,
+ entryOp.value, msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
+ entryOp.filterRoutingInfo, msg.bridgeContext, null, entryOp.versionTag,
entryOp.tailKey);
try {
if (entryOp.filterRoutingInfo != null) {
eei.setLocalFilterInfo(
- entryOp.filterRoutingInfo.getFilterInfo(this.internalRegion.getCache().getMyId()));
+ entryOp.filterRoutingInfo.getFilterInfo(internalRegion.getCache().getMyId()));
}
if (isDuplicate) {
eei.setPossibleDuplicate(true);
}
if (logger.isDebugEnabled()) {
logger.debug("invoking transactional callbacks for {} key={} needsUnlock={} event={}",
- entryOp.op, entryOp.key, this.needsUnlock, eei);
+ entryOp.op, entryOp.key, needsUnlock, eei);
}
// we reach this spot because the event is either delivered to this member
// as an "adjunct" message or because the bucket was being created when
// the message was sent and already reflects the change caused by this event.
// In the latter case we need to invoke listeners
final boolean skipListeners = !isDuplicate;
- eei.invokeCallbacks(this.internalRegion, skipListeners, true);
+ eei.invokeCallbacks(internalRegion, skipListeners, true);
} finally {
eei.release();
}
- return;
}
}
boolean isEmpty() {
- return this.opKeys == null;
+ return opKeys == null;
}
boolean needsAck() {
- return this.internalRegion.getScope().isDistributedAck();
+ return internalRegion.getScope().isDistributedAck();
}
void addOp(Object key, TXEntryState entry) {
- if (this.opKeys == null) {
- this.opKeys = new ArrayList(this.maxSize);
- this.opEntries = new ArrayList(this.maxSize);
+ if (opKeys == null) {
+ opKeys = new ArrayList<>(maxSize);
+ opEntries = new ArrayList<>(maxSize);
}
- this.opKeys.add(key);
- this.opEntries.add(entry);
+ opKeys.add(key);
+ opEntries.add(entry);
}
public boolean isForceFireEvent(DistributionManager dm) {
LocalRegion r = getRegionByPath(dm, regionPath);
- if (r instanceof PartitionedRegion || (r != null && r.isUsedForPartitionedRegionBucket())) {
- return false;
- }
- return true;
+ return !(r instanceof PartitionedRegion) && (r == null || !r
+ .isUsedForPartitionedRegionBucket());
}
public void fromData(DataInput in, boolean hasShadowKey)
throws IOException, ClassNotFoundException {
- this.regionPath = DataSerializer.readString(in);
- this.parentRegionPath = DataSerializer.readString(in);
+ regionPath = DataSerializer.readString(in);
+ parentRegionPath = DataSerializer.readString(in);
int size = in.readInt();
if (size > 0) {
- this.opKeys = new ArrayList(size);
- this.opEntries = new ArrayList(size);
+ opKeys = new ArrayList<>(size);
+ opEntries = new ArrayList<>(size);
final boolean largeModCount = in.readBoolean();
- this.memberId = DataSerializer.readObject(in);
+ memberId = DataSerializer.readObject(in);
for (int i = 0; i < size; i++) {
FarSideEntryOp entryOp = new FarSideEntryOp();
// shadowkey is not being sent to clients
entryOp.fromData(in, largeModCount, hasShadowKey);
- if (entryOp.versionTag != null && this.memberId != null) {
- entryOp.versionTag.setMemberID(this.memberId);
+ if (entryOp.versionTag != null && memberId != null) {
+ entryOp.versionTag.setMemberID(memberId);
}
- this.msg.addFarSideEntryOp(entryOp);
- this.opKeys.add(entryOp.key);
- this.opEntries.add(entryOp);
+ msg.addFarSideEntryOp(entryOp);
+ opKeys.add(entryOp.key);
+ opEntries.add(entryOp);
}
}
}
@Override
public String toString() {
StringBuilder result = new StringBuilder(64);
- if (this.regionPath != null) {
- result.append(this.regionPath);
+ if (regionPath != null) {
+ result.append(regionPath);
} else {
- result.append(this.internalRegion.getFullPath());
+ result.append(internalRegion.getFullPath());
}
- if (this.refCount > 0) {
- result.append(" refCount=").append(this.refCount);
+ if (refCount > 0) {
+ result.append(" refCount=").append(refCount);
}
return result.toString();
}
private void basicToData(DataOutput out,
SerializationContext context,
boolean useShadowKey) throws IOException {
- if (this.internalRegion != null) {
- DataSerializer.writeString(this.internalRegion.getFullPath(), out);
- if (this.internalRegion instanceof BucketRegion) {
+ if (internalRegion != null) {
+ DataSerializer.writeString(internalRegion.getFullPath(), out);
+ if (internalRegion instanceof BucketRegion) {
DataSerializer.writeString(
- ((Bucket) this.internalRegion).getPartitionedRegion().getFullPath(), out);
+ ((Bucket) internalRegion).getPartitionedRegion().getFullPath(), out);
} else {
DataSerializer.writeString(null, out);
}
} else {
- DataSerializer.writeString(this.regionPath, out);
- DataSerializer.writeString(this.parentRegionPath, out);
+ DataSerializer.writeString(regionPath, out);
+ DataSerializer.writeString(parentRegionPath, out);
}
- if (isEmpty() || this.opKeys.size() == 0) {
+ if (isEmpty() || opKeys.size() == 0) {
out.writeInt(0);
} else {
- int size = this.opKeys.size();
+ int size = opKeys.size();
out.writeInt(size);
final boolean largeModCount;
- if (this.msg.txState != null) {
- largeModCount = this.msg.txState.needsLargeModCount();
+ if (msg.txState != null) {
+ largeModCount = msg.txState.needsLargeModCount();
} else {
- largeModCount = this.msg.needsLargeModCount;
+ largeModCount = msg.needsLargeModCount;
}
out.writeBoolean(largeModCount);
- final boolean sendVersionTags =
- this.msg.clientVersion == null
- || KnownVersion.GFE_70.compareTo(this.msg.clientVersion) <= 0;
- if (sendVersionTags) {
- VersionSource member = this.memberId;
- if (member == null) {
- if (this.internalRegion == null) {
- Assert.assertTrue(this.msg.txState == null);
- } else {
- member = this.internalRegion.getVersionMember();
- }
+ VersionSource member = memberId;
Review comment:
Here we have completely removed the check for msg.clientVersion for null. Looking at the code there are some places where this value is set to null like in CommitCommand. Now the member version source is always sent, is this behavior change correct?
--
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 #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r658945611
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/TXCommitMessage.java
##########
@@ -1384,153 +1323,145 @@ protected void txApplyEntryOpAdjunctOnly(FarSideEntryOp entryOp) {
*/
@Released
EntryEventImpl eei =
- txCallbackEventFactory.createCallbackEvent(this.internalRegion, entryOp.op,
+ txCallbackEventFactory.createCallbackEvent(internalRegion, entryOp.op,
entryOp.key,
- entryOp.value, this.msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
- entryOp.filterRoutingInfo, this.msg.bridgeContext, null, entryOp.versionTag,
+ entryOp.value, msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
+ entryOp.filterRoutingInfo, msg.bridgeContext, null, entryOp.versionTag,
entryOp.tailKey);
try {
if (entryOp.filterRoutingInfo != null) {
eei.setLocalFilterInfo(
- entryOp.filterRoutingInfo.getFilterInfo(this.internalRegion.getCache().getMyId()));
+ entryOp.filterRoutingInfo.getFilterInfo(internalRegion.getCache().getMyId()));
}
if (isDuplicate) {
eei.setPossibleDuplicate(true);
}
if (logger.isDebugEnabled()) {
logger.debug("invoking transactional callbacks for {} key={} needsUnlock={} event={}",
- entryOp.op, entryOp.key, this.needsUnlock, eei);
+ entryOp.op, entryOp.key, needsUnlock, eei);
}
// we reach this spot because the event is either delivered to this member
// as an "adjunct" message or because the bucket was being created when
// the message was sent and already reflects the change caused by this event.
// In the latter case we need to invoke listeners
final boolean skipListeners = !isDuplicate;
- eei.invokeCallbacks(this.internalRegion, skipListeners, true);
+ eei.invokeCallbacks(internalRegion, skipListeners, true);
} finally {
eei.release();
}
- return;
}
}
boolean isEmpty() {
- return this.opKeys == null;
+ return opKeys == null;
}
boolean needsAck() {
- return this.internalRegion.getScope().isDistributedAck();
+ return internalRegion.getScope().isDistributedAck();
}
void addOp(Object key, TXEntryState entry) {
- if (this.opKeys == null) {
- this.opKeys = new ArrayList(this.maxSize);
- this.opEntries = new ArrayList(this.maxSize);
+ if (opKeys == null) {
+ opKeys = new ArrayList<>(maxSize);
+ opEntries = new ArrayList<>(maxSize);
}
- this.opKeys.add(key);
- this.opEntries.add(entry);
+ opKeys.add(key);
+ opEntries.add(entry);
}
public boolean isForceFireEvent(DistributionManager dm) {
LocalRegion r = getRegionByPath(dm, regionPath);
- if (r instanceof PartitionedRegion || (r != null && r.isUsedForPartitionedRegionBucket())) {
- return false;
- }
- return true;
+ return !(r instanceof PartitionedRegion) && (r == null || !r
+ .isUsedForPartitionedRegionBucket());
}
public void fromData(DataInput in, boolean hasShadowKey)
throws IOException, ClassNotFoundException {
- this.regionPath = DataSerializer.readString(in);
- this.parentRegionPath = DataSerializer.readString(in);
+ regionPath = DataSerializer.readString(in);
+ parentRegionPath = DataSerializer.readString(in);
int size = in.readInt();
if (size > 0) {
- this.opKeys = new ArrayList(size);
- this.opEntries = new ArrayList(size);
+ opKeys = new ArrayList<>(size);
+ opEntries = new ArrayList<>(size);
final boolean largeModCount = in.readBoolean();
- this.memberId = DataSerializer.readObject(in);
+ memberId = DataSerializer.readObject(in);
for (int i = 0; i < size; i++) {
FarSideEntryOp entryOp = new FarSideEntryOp();
// shadowkey is not being sent to clients
entryOp.fromData(in, largeModCount, hasShadowKey);
- if (entryOp.versionTag != null && this.memberId != null) {
- entryOp.versionTag.setMemberID(this.memberId);
+ if (entryOp.versionTag != null && memberId != null) {
+ entryOp.versionTag.setMemberID(memberId);
}
- this.msg.addFarSideEntryOp(entryOp);
- this.opKeys.add(entryOp.key);
- this.opEntries.add(entryOp);
+ msg.addFarSideEntryOp(entryOp);
+ opKeys.add(entryOp.key);
+ opEntries.add(entryOp);
}
}
}
@Override
public String toString() {
StringBuilder result = new StringBuilder(64);
- if (this.regionPath != null) {
- result.append(this.regionPath);
+ if (regionPath != null) {
+ result.append(regionPath);
} else {
- result.append(this.internalRegion.getFullPath());
+ result.append(internalRegion.getFullPath());
}
- if (this.refCount > 0) {
- result.append(" refCount=").append(this.refCount);
+ if (refCount > 0) {
+ result.append(" refCount=").append(refCount);
}
return result.toString();
}
private void basicToData(DataOutput out,
SerializationContext context,
boolean useShadowKey) throws IOException {
- if (this.internalRegion != null) {
- DataSerializer.writeString(this.internalRegion.getFullPath(), out);
- if (this.internalRegion instanceof BucketRegion) {
+ if (internalRegion != null) {
+ DataSerializer.writeString(internalRegion.getFullPath(), out);
+ if (internalRegion instanceof BucketRegion) {
DataSerializer.writeString(
- ((Bucket) this.internalRegion).getPartitionedRegion().getFullPath(), out);
+ ((Bucket) internalRegion).getPartitionedRegion().getFullPath(), out);
} else {
DataSerializer.writeString(null, out);
}
} else {
- DataSerializer.writeString(this.regionPath, out);
- DataSerializer.writeString(this.parentRegionPath, out);
+ DataSerializer.writeString(regionPath, out);
+ DataSerializer.writeString(parentRegionPath, out);
}
- if (isEmpty() || this.opKeys.size() == 0) {
+ if (isEmpty() || opKeys.size() == 0) {
out.writeInt(0);
} else {
- int size = this.opKeys.size();
+ int size = opKeys.size();
out.writeInt(size);
final boolean largeModCount;
- if (this.msg.txState != null) {
- largeModCount = this.msg.txState.needsLargeModCount();
+ if (msg.txState != null) {
+ largeModCount = msg.txState.needsLargeModCount();
} else {
- largeModCount = this.msg.needsLargeModCount;
+ largeModCount = msg.needsLargeModCount;
}
out.writeBoolean(largeModCount);
- final boolean sendVersionTags =
- this.msg.clientVersion == null
- || KnownVersion.GFE_70.compareTo(this.msg.clientVersion) <= 0;
- if (sendVersionTags) {
- VersionSource member = this.memberId;
- if (member == null) {
- if (this.internalRegion == null) {
- Assert.assertTrue(this.msg.txState == null);
- } else {
- member = this.internalRegion.getVersionMember();
- }
+ VersionSource member = memberId;
Review comment:
It would send the `VersionSource` if it didn't know the `clientVersion` or it was newer than v7.0. In this case the latter part is always true so the check for unknown or `null` `clientVersion` is redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on pull request #6500:
URL: https://github.com/apache/geode/pull/6500#issuecomment-868739736
Also, I would request refactorings to be kept in separate PRs.
- The caption JIRA mentions that this GEODE-8870 ticket is for removal of obsolete GFE_70 code but has GEODE-6588 along with it which has other major refactorings.
- Keeping the CL shorter will have faster approval / thorough review as the reviewer list smaller
- We view the final set of files changed as it is more convenient than looking at each and every commit individually, as the same file may have been modified in multiple commits.
--
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 #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r636254606
##########
File path: geode-core/src/test/java/org/apache/geode/internal/DataSerializableJUnitTest.java
##########
@@ -1166,13 +1166,13 @@ public void testVersionedDataSerializable() throws Exception {
VersionedDataSerializableImpl ds = new VersionedDataSerializableImpl(getRandom());
VersionedDataOutputStream v =
- new VersionedDataOutputStream(this.baos, KnownVersion.GFE_70);
+ new VersionedDataOutputStream(this.baos, KnownVersion.GEODE_1_11_0);
Review comment:
No, they are just arbitrary choices in two different versions to maintain the behavior of the test using versions that aren't being removed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #6500:
URL: https://github.com/apache/geode/pull/6500#issuecomment-854137953
I see lot more refactoring changes than the fix for actual issue. I agree and believe in incremental refactoring as we come across the opportunity; but here I see lot more refactoring changes; even in non-related files (where the actual change is not expected); it will be nice to keep those changes minimal and focused on the issue being fixed...just my thoughts and I am fine with other way too...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #6500:
URL: https://github.com/apache/geode/pull/6500#issuecomment-854137953
I see lot more refactoring changes than the fix for actual issue. I agree and believe in incremental refactoring as we come across the opportunity; but here I see lot more refactoring changes; even in non-related files (where the actual change is not expected); it will be nice to keep those changes minimal and focused on the issue being fixed...just my thoughts and I am fine with other way too...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a change in pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r658939295
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/TXCommitMessage.java
##########
@@ -1384,153 +1323,145 @@ protected void txApplyEntryOpAdjunctOnly(FarSideEntryOp entryOp) {
*/
@Released
EntryEventImpl eei =
- txCallbackEventFactory.createCallbackEvent(this.internalRegion, entryOp.op,
+ txCallbackEventFactory.createCallbackEvent(internalRegion, entryOp.op,
entryOp.key,
- entryOp.value, this.msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
- entryOp.filterRoutingInfo, this.msg.bridgeContext, null, entryOp.versionTag,
+ entryOp.value, msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
+ entryOp.filterRoutingInfo, msg.bridgeContext, null, entryOp.versionTag,
entryOp.tailKey);
try {
if (entryOp.filterRoutingInfo != null) {
eei.setLocalFilterInfo(
- entryOp.filterRoutingInfo.getFilterInfo(this.internalRegion.getCache().getMyId()));
+ entryOp.filterRoutingInfo.getFilterInfo(internalRegion.getCache().getMyId()));
}
if (isDuplicate) {
eei.setPossibleDuplicate(true);
}
if (logger.isDebugEnabled()) {
logger.debug("invoking transactional callbacks for {} key={} needsUnlock={} event={}",
- entryOp.op, entryOp.key, this.needsUnlock, eei);
+ entryOp.op, entryOp.key, needsUnlock, eei);
}
// we reach this spot because the event is either delivered to this member
// as an "adjunct" message or because the bucket was being created when
// the message was sent and already reflects the change caused by this event.
// In the latter case we need to invoke listeners
final boolean skipListeners = !isDuplicate;
- eei.invokeCallbacks(this.internalRegion, skipListeners, true);
+ eei.invokeCallbacks(internalRegion, skipListeners, true);
} finally {
eei.release();
}
- return;
}
}
boolean isEmpty() {
- return this.opKeys == null;
+ return opKeys == null;
}
boolean needsAck() {
- return this.internalRegion.getScope().isDistributedAck();
+ return internalRegion.getScope().isDistributedAck();
}
void addOp(Object key, TXEntryState entry) {
- if (this.opKeys == null) {
- this.opKeys = new ArrayList(this.maxSize);
- this.opEntries = new ArrayList(this.maxSize);
+ if (opKeys == null) {
+ opKeys = new ArrayList<>(maxSize);
+ opEntries = new ArrayList<>(maxSize);
}
- this.opKeys.add(key);
- this.opEntries.add(entry);
+ opKeys.add(key);
+ opEntries.add(entry);
}
public boolean isForceFireEvent(DistributionManager dm) {
LocalRegion r = getRegionByPath(dm, regionPath);
- if (r instanceof PartitionedRegion || (r != null && r.isUsedForPartitionedRegionBucket())) {
- return false;
- }
- return true;
+ return !(r instanceof PartitionedRegion) && (r == null || !r
+ .isUsedForPartitionedRegionBucket());
}
public void fromData(DataInput in, boolean hasShadowKey)
throws IOException, ClassNotFoundException {
- this.regionPath = DataSerializer.readString(in);
- this.parentRegionPath = DataSerializer.readString(in);
+ regionPath = DataSerializer.readString(in);
+ parentRegionPath = DataSerializer.readString(in);
int size = in.readInt();
if (size > 0) {
- this.opKeys = new ArrayList(size);
- this.opEntries = new ArrayList(size);
+ opKeys = new ArrayList<>(size);
+ opEntries = new ArrayList<>(size);
final boolean largeModCount = in.readBoolean();
- this.memberId = DataSerializer.readObject(in);
+ memberId = DataSerializer.readObject(in);
for (int i = 0; i < size; i++) {
FarSideEntryOp entryOp = new FarSideEntryOp();
// shadowkey is not being sent to clients
entryOp.fromData(in, largeModCount, hasShadowKey);
- if (entryOp.versionTag != null && this.memberId != null) {
- entryOp.versionTag.setMemberID(this.memberId);
+ if (entryOp.versionTag != null && memberId != null) {
+ entryOp.versionTag.setMemberID(memberId);
}
- this.msg.addFarSideEntryOp(entryOp);
- this.opKeys.add(entryOp.key);
- this.opEntries.add(entryOp);
+ msg.addFarSideEntryOp(entryOp);
+ opKeys.add(entryOp.key);
+ opEntries.add(entryOp);
}
}
}
@Override
public String toString() {
StringBuilder result = new StringBuilder(64);
- if (this.regionPath != null) {
- result.append(this.regionPath);
+ if (regionPath != null) {
+ result.append(regionPath);
} else {
- result.append(this.internalRegion.getFullPath());
+ result.append(internalRegion.getFullPath());
}
- if (this.refCount > 0) {
- result.append(" refCount=").append(this.refCount);
+ if (refCount > 0) {
+ result.append(" refCount=").append(refCount);
}
return result.toString();
}
private void basicToData(DataOutput out,
SerializationContext context,
boolean useShadowKey) throws IOException {
- if (this.internalRegion != null) {
- DataSerializer.writeString(this.internalRegion.getFullPath(), out);
- if (this.internalRegion instanceof BucketRegion) {
+ if (internalRegion != null) {
+ DataSerializer.writeString(internalRegion.getFullPath(), out);
+ if (internalRegion instanceof BucketRegion) {
DataSerializer.writeString(
- ((Bucket) this.internalRegion).getPartitionedRegion().getFullPath(), out);
+ ((Bucket) internalRegion).getPartitionedRegion().getFullPath(), out);
} else {
DataSerializer.writeString(null, out);
}
} else {
- DataSerializer.writeString(this.regionPath, out);
- DataSerializer.writeString(this.parentRegionPath, out);
+ DataSerializer.writeString(regionPath, out);
+ DataSerializer.writeString(parentRegionPath, out);
}
- if (isEmpty() || this.opKeys.size() == 0) {
+ if (isEmpty() || opKeys.size() == 0) {
out.writeInt(0);
} else {
- int size = this.opKeys.size();
+ int size = opKeys.size();
out.writeInt(size);
final boolean largeModCount;
- if (this.msg.txState != null) {
- largeModCount = this.msg.txState.needsLargeModCount();
+ if (msg.txState != null) {
+ largeModCount = msg.txState.needsLargeModCount();
} else {
- largeModCount = this.msg.needsLargeModCount;
+ largeModCount = msg.needsLargeModCount;
}
out.writeBoolean(largeModCount);
- final boolean sendVersionTags =
- this.msg.clientVersion == null
- || KnownVersion.GFE_70.compareTo(this.msg.clientVersion) <= 0;
- if (sendVersionTags) {
- VersionSource member = this.memberId;
- if (member == null) {
- if (this.internalRegion == null) {
- Assert.assertTrue(this.msg.txState == null);
- } else {
- member = this.internalRegion.getVersionMember();
- }
+ VersionSource member = memberId;
+ if (member == null) {
+ if (internalRegion == null) {
+ Assert.assertTrue(msg.txState == null);
+ } else {
+ member = internalRegion.getVersionMember();
}
- DataSerializer.writeObject(member, out);
}
+ DataSerializer.writeObject(member, out);
for (int i = 0; i < size; i++) {
- DataSerializer.writeObject(this.opKeys.get(i), out);
- if (this.msg.txState != null) {
+ DataSerializer.writeObject(opKeys.get(i), out);
+ if (msg.txState != null) {
/* we are still on tx node and have the entry state */
- ((TXEntryState) this.opEntries.get(i)).toFarSideData(out, context, largeModCount,
- sendVersionTags, useShadowKey);
+ ((TXEntryState) opEntries.get(i)).toFarSideData(out, context, largeModCount,
+ true, useShadowKey);
Review comment:
Now sendVersionTags is always set to to true irrespective of if msg.clientVersion is null or not.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/TXCommitMessage.java
##########
@@ -1384,153 +1323,145 @@ protected void txApplyEntryOpAdjunctOnly(FarSideEntryOp entryOp) {
*/
@Released
EntryEventImpl eei =
- txCallbackEventFactory.createCallbackEvent(this.internalRegion, entryOp.op,
+ txCallbackEventFactory.createCallbackEvent(internalRegion, entryOp.op,
entryOp.key,
- entryOp.value, this.msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
- entryOp.filterRoutingInfo, this.msg.bridgeContext, null, entryOp.versionTag,
+ entryOp.value, msg.txIdent, txEvent, getEventId(entryOp), entryOp.callbackArg,
+ entryOp.filterRoutingInfo, msg.bridgeContext, null, entryOp.versionTag,
entryOp.tailKey);
try {
if (entryOp.filterRoutingInfo != null) {
eei.setLocalFilterInfo(
- entryOp.filterRoutingInfo.getFilterInfo(this.internalRegion.getCache().getMyId()));
+ entryOp.filterRoutingInfo.getFilterInfo(internalRegion.getCache().getMyId()));
}
if (isDuplicate) {
eei.setPossibleDuplicate(true);
}
if (logger.isDebugEnabled()) {
logger.debug("invoking transactional callbacks for {} key={} needsUnlock={} event={}",
- entryOp.op, entryOp.key, this.needsUnlock, eei);
+ entryOp.op, entryOp.key, needsUnlock, eei);
}
// we reach this spot because the event is either delivered to this member
// as an "adjunct" message or because the bucket was being created when
// the message was sent and already reflects the change caused by this event.
// In the latter case we need to invoke listeners
final boolean skipListeners = !isDuplicate;
- eei.invokeCallbacks(this.internalRegion, skipListeners, true);
+ eei.invokeCallbacks(internalRegion, skipListeners, true);
} finally {
eei.release();
}
- return;
}
}
boolean isEmpty() {
- return this.opKeys == null;
+ return opKeys == null;
}
boolean needsAck() {
- return this.internalRegion.getScope().isDistributedAck();
+ return internalRegion.getScope().isDistributedAck();
}
void addOp(Object key, TXEntryState entry) {
- if (this.opKeys == null) {
- this.opKeys = new ArrayList(this.maxSize);
- this.opEntries = new ArrayList(this.maxSize);
+ if (opKeys == null) {
+ opKeys = new ArrayList<>(maxSize);
+ opEntries = new ArrayList<>(maxSize);
}
- this.opKeys.add(key);
- this.opEntries.add(entry);
+ opKeys.add(key);
+ opEntries.add(entry);
}
public boolean isForceFireEvent(DistributionManager dm) {
LocalRegion r = getRegionByPath(dm, regionPath);
- if (r instanceof PartitionedRegion || (r != null && r.isUsedForPartitionedRegionBucket())) {
- return false;
- }
- return true;
+ return !(r instanceof PartitionedRegion) && (r == null || !r
+ .isUsedForPartitionedRegionBucket());
}
public void fromData(DataInput in, boolean hasShadowKey)
throws IOException, ClassNotFoundException {
- this.regionPath = DataSerializer.readString(in);
- this.parentRegionPath = DataSerializer.readString(in);
+ regionPath = DataSerializer.readString(in);
+ parentRegionPath = DataSerializer.readString(in);
int size = in.readInt();
if (size > 0) {
- this.opKeys = new ArrayList(size);
- this.opEntries = new ArrayList(size);
+ opKeys = new ArrayList<>(size);
+ opEntries = new ArrayList<>(size);
final boolean largeModCount = in.readBoolean();
- this.memberId = DataSerializer.readObject(in);
+ memberId = DataSerializer.readObject(in);
for (int i = 0; i < size; i++) {
FarSideEntryOp entryOp = new FarSideEntryOp();
// shadowkey is not being sent to clients
entryOp.fromData(in, largeModCount, hasShadowKey);
- if (entryOp.versionTag != null && this.memberId != null) {
- entryOp.versionTag.setMemberID(this.memberId);
+ if (entryOp.versionTag != null && memberId != null) {
+ entryOp.versionTag.setMemberID(memberId);
}
- this.msg.addFarSideEntryOp(entryOp);
- this.opKeys.add(entryOp.key);
- this.opEntries.add(entryOp);
+ msg.addFarSideEntryOp(entryOp);
+ opKeys.add(entryOp.key);
+ opEntries.add(entryOp);
}
}
}
@Override
public String toString() {
StringBuilder result = new StringBuilder(64);
- if (this.regionPath != null) {
- result.append(this.regionPath);
+ if (regionPath != null) {
+ result.append(regionPath);
} else {
- result.append(this.internalRegion.getFullPath());
+ result.append(internalRegion.getFullPath());
}
- if (this.refCount > 0) {
- result.append(" refCount=").append(this.refCount);
+ if (refCount > 0) {
+ result.append(" refCount=").append(refCount);
}
return result.toString();
}
private void basicToData(DataOutput out,
SerializationContext context,
boolean useShadowKey) throws IOException {
- if (this.internalRegion != null) {
- DataSerializer.writeString(this.internalRegion.getFullPath(), out);
- if (this.internalRegion instanceof BucketRegion) {
+ if (internalRegion != null) {
+ DataSerializer.writeString(internalRegion.getFullPath(), out);
+ if (internalRegion instanceof BucketRegion) {
DataSerializer.writeString(
- ((Bucket) this.internalRegion).getPartitionedRegion().getFullPath(), out);
+ ((Bucket) internalRegion).getPartitionedRegion().getFullPath(), out);
} else {
DataSerializer.writeString(null, out);
}
} else {
- DataSerializer.writeString(this.regionPath, out);
- DataSerializer.writeString(this.parentRegionPath, out);
+ DataSerializer.writeString(regionPath, out);
+ DataSerializer.writeString(parentRegionPath, out);
}
- if (isEmpty() || this.opKeys.size() == 0) {
+ if (isEmpty() || opKeys.size() == 0) {
out.writeInt(0);
} else {
- int size = this.opKeys.size();
+ int size = opKeys.size();
out.writeInt(size);
final boolean largeModCount;
- if (this.msg.txState != null) {
- largeModCount = this.msg.txState.needsLargeModCount();
+ if (msg.txState != null) {
+ largeModCount = msg.txState.needsLargeModCount();
} else {
- largeModCount = this.msg.needsLargeModCount;
+ largeModCount = msg.needsLargeModCount;
}
out.writeBoolean(largeModCount);
- final boolean sendVersionTags =
- this.msg.clientVersion == null
- || KnownVersion.GFE_70.compareTo(this.msg.clientVersion) <= 0;
- if (sendVersionTags) {
- VersionSource member = this.memberId;
- if (member == null) {
- if (this.internalRegion == null) {
- Assert.assertTrue(this.msg.txState == null);
- } else {
- member = this.internalRegion.getVersionMember();
- }
+ VersionSource member = memberId;
Review comment:
Here we have completely removed the check for msg.clientVersion for null. Looking at the code there are some places where this valus is set to nul like in CommitCommand. Now the member version source is always sent, is this behavior change correct?
--
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 #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r636328156
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -5723,89 +5590,79 @@ public DirectoryHolder getDirectoryHolder() {
* @return long value indicating the current size of the oplog.
*/
long getOplogSize() {
- return this.crf.currSize + this.drf.currSize;
+ return crf.currSize + drf.currSize;
}
boolean isOplogEmpty() {
- return this.crf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE)
- && this.drf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE);
+ return crf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE)
+ && drf.currSize <= (OPLOG_DISK_STORE_REC_SIZE + OPLOG_MAGIC_SEQ_REC_SIZE);
}
void incLiveCount() {
- this.totalLiveCount.incrementAndGet();
+ totalLiveCount.incrementAndGet();
}
private void decLiveCount() {
- this.totalLiveCount.decrementAndGet();
+ totalLiveCount.decrementAndGet();
}
/**
* Return true if a record (crf or drf) has been added to this oplog
*/
boolean hasBeenUsed() {
- return this.hasDeletes.get() || this.totalCount.get() > 0;
+ return hasDeletes.get() || totalCount.get() > 0;
}
void incTotalCount() {
if (!isPhase2()) {
- this.totalCount.incrementAndGet();
+ totalCount.incrementAndGet();
}
}
private void finishedAppending() {
// No need to get the backup lock prior to synchronizing (correct lock order) since the
// synchronized block does not attempt to get the backup lock (incorrect lock order)
- synchronized (this.lock/* crf */) {
- this.doneAppending = true;
+ synchronized (lock/* crf */) {
+ doneAppending = true;
}
handleNoLiveValues();
// I'm deadcoding the following because it is not safe unless we change to
// always recover values. If we don't recover values then
// an oplog we recovered from may still need to fault values in from memory.
Review comment:
Sounds good. I'll approve the PR now, on the assumption that the commit will get pushed before it's merged.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6500: GEODE-8870: Removes GFE_70.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6500:
URL: https://github.com/apache/geode/pull/6500#discussion_r636963137
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/persistence/DiskInitFileParser.java
##########
@@ -567,24 +568,20 @@ private void readEndOfRecord(DataInput di) throws IOException {
}
}
- private PersistentMemberID readPMID(CountingDataInputStream dis, KnownVersion gfversion)
+ private PersistentMemberID readPMID(CountingDataInputStream dis)
throws IOException, ClassNotFoundException {
int len = dis.readInt();
byte[] buf = new byte[len];
dis.readFully(buf);
- return bytesToPMID(buf, gfversion);
+ return bytesToPMID(buf);
}
- private PersistentMemberID bytesToPMID(byte[] bytes, KnownVersion gfversion)
+ private PersistentMemberID bytesToPMID(byte[] bytes)
throws IOException, ClassNotFoundException {
ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
DataInputStream dis = new DataInputStream(bais);
PersistentMemberID result = new PersistentMemberID();
- if (KnownVersion.GFE_70.compareTo(gfversion) > 0) {
- result._fromData662(dis);
- } else {
- InternalDataSerializer.invokeFromData(result, dis);
- }
+ InternalDataSerializer.invokeFromData(result, dis);
Review comment:
I think if this method needs versioning support added it should come when there is a version change here or a separate PR to add versioning support and tests. The only change here was to pull out the obsolete version check which left us with the original un-versioned from data call.
Going through all these cleanups and digging deep into areas of code I haven't been I have noticed there is inconsistent use of `VersionedDataInputStream` and `VersionedDataSerializable`. I wonder if we should have an effort to normalize on the versioned versions (🥁) and deprecate the older stuff.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org