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/13 10:19:27 UTC
[GitHub] [geode] mivanac opened a new pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
mivanac opened a new pull request #6474:
URL: https://github.com/apache/geode/pull/6474
…ment when is peeked
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] mivanac commented on a change in pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #6474:
URL: https://github.com/apache/geode/pull/6474#discussion_r639199111
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -423,8 +428,14 @@ public Object peek() {
}
key = this.eventSeqNumDeque.peekFirst();
if (key != null) {
+ boolean setDuplicate = markAsDuplicate.remove(key);
+
object = optimalGet(key);
Review comment:
markEventsAsDuplicate() marks all events in queue as duplicate at the moment that bucket is becoming primary(and getNoLRU calls getValueInVM or getValueOnDiskOrBuffer).
With new changes, events in queue will be marked as duplicate at the moment that they are peeked/unqueued (optimalGet calls getValueInVMOrDiskWithoutFaultIn)
--
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] mivanac commented on a change in pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #6474:
URL: https://github.com/apache/geode/pull/6474#discussion_r639449206
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -203,8 +206,7 @@ private void destroyFailedBatchRemovalMessageKeys() {
@Override
public void beforeAcquiringPrimaryState() {
- Iterator<Object> itr = eventSeqNumDeque.iterator();
- markEventsAsDuplicate(itr);
+ markAsDuplicate.addAll(Arrays.asList(eventSeqNumDeque.toArray()));
Review comment:
Thanx. Updated.
--
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] mivanac merged pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
Posted by GitBox <gi...@apache.org>.
mivanac merged pull request #6474:
URL: https://github.com/apache/geode/pull/6474
--
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] mivanac commented on a change in pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #6474:
URL: https://github.com/apache/geode/pull/6474#discussion_r639449398
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -203,8 +206,7 @@ private void destroyFailedBatchRemovalMessageKeys() {
@Override
public void beforeAcquiringPrimaryState() {
- Iterator<Object> itr = eventSeqNumDeque.iterator();
- markEventsAsDuplicate(itr);
Review comment:
Deleted.
--
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] mivanac commented on a change in pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #6474:
URL: https://github.com/apache/geode/pull/6474#discussion_r639224907
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -203,8 +206,7 @@ private void destroyFailedBatchRemovalMessageKeys() {
@Override
public void beforeAcquiringPrimaryState() {
- Iterator<Object> itr = eventSeqNumDeque.iterator();
- markEventsAsDuplicate(itr);
Review comment:
BucketAdvisor class, method acquiredPrimaryLock() calls
if (br instanceof BucketRegion) {
((BucketRegion) br).beforeAcquiringPrimaryState();
}
--
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] DonalEvans commented on a change in pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6474:
URL: https://github.com/apache/geode/pull/6474#discussion_r639226220
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -203,8 +206,7 @@ private void destroyFailedBatchRemovalMessageKeys() {
@Override
public void beforeAcquiringPrimaryState() {
- Iterator<Object> itr = eventSeqNumDeque.iterator();
- markEventsAsDuplicate(itr);
Review comment:
Ah, I'm sorry, I should have been clearer in the initial comment. The method that I'm referring to is `markEventsAsDuplicate()`.
--
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] mivanac commented on a change in pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #6474:
URL: https://github.com/apache/geode/pull/6474#discussion_r639208182
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -203,8 +206,7 @@ private void destroyFailedBatchRemovalMessageKeys() {
@Override
public void beforeAcquiringPrimaryState() {
- Iterator<Object> itr = eventSeqNumDeque.iterator();
- markEventsAsDuplicate(itr);
Review comment:
This method is used to current queue copy to marked list, so we can not remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6474:
URL: https://github.com/apache/geode/pull/6474#discussion_r639129906
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -203,8 +206,7 @@ private void destroyFailedBatchRemovalMessageKeys() {
@Override
public void beforeAcquiringPrimaryState() {
- Iterator<Object> itr = eventSeqNumDeque.iterator();
- markEventsAsDuplicate(itr);
+ markAsDuplicate.addAll(Arrays.asList(eventSeqNumDeque.toArray()));
Review comment:
I think that this can just be
```
markAsDuplicate.addAll(eventSeqNumDeque);
```
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -203,8 +206,7 @@ private void destroyFailedBatchRemovalMessageKeys() {
@Override
public void beforeAcquiringPrimaryState() {
- Iterator<Object> itr = eventSeqNumDeque.iterator();
- markEventsAsDuplicate(itr);
Review comment:
With this change, this method is no longer used, and so can be removed. However, some debug-level logging is lost as a result, so maybe that should be moved to where you're now calling `setPossibleDuplicate()`.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -423,8 +428,14 @@ public Object peek() {
}
key = this.eventSeqNumDeque.peekFirst();
if (key != null) {
+ boolean setDuplicate = markAsDuplicate.remove(key);
+
object = optimalGet(key);
Review comment:
The old `markEventsAsDuplicate()` method used `getNoLRU()` to retrieve the object and determine if `setPossibleDuplicate()` should be called on it. Will using `optimalGet()` here result in the same behaviour?
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/BucketRegionQueueJUnitTest.java
##########
@@ -199,4 +256,34 @@ GatewaySenderEventImpl createMockGatewaySenderEvent(Object key, TransactionId tI
when(event.getKey()).thenReturn(key);
return event;
}
+
+ private GatewaySenderEventImpl createGatewaySenderEvent(LocalRegion lr, Operation operation,
+ Object key, Object value, long threadId, long sequenceId)
+ throws Exception {
+ when(lr.getKeyInfo(key, value, null)).thenReturn(new KeyInfo(key, null, null));
+ when(lr.getTXId()).thenReturn(null);
+
+
+
Review comment:
Some unnecessary line breaks here.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -668,4 +679,27 @@ public boolean isReadyForPeek() {
&& !this.eventSeqNumDeque.isEmpty() && getBucketAdvisor().isPrimary();
}
+ List<Object> getQueueList() {
+ getInitializationLock().readLock().lock();
+ try {
+ if (this.getPartitionedRegion().isDestroyed()) {
+ throw new BucketRegionQueueUnavailableException();
+ }
+ List<Object> elementsInQueue = new ArrayList<>();
+ Iterator<Object> it = this.eventSeqNumDeque.iterator();
+ while (it.hasNext()) {
+ Object key = it.next();
+ Object event = optimalGet(key);
+
+ if (!(event instanceof InternalGatewayQueueEvent)) {
+ continue;
+ }
+ elementsInQueue.add(event);
+ }
+ return elementsInQueue;
Review comment:
Purely personal preference, but this can be simplified significantly using streams:
```
return eventSeqNumDeque.stream()
.map(this::optimalGet)
.filter(o -> o instanceof InternalGatewayQueueEvent)
.collect(Collectors.toList());
```
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/BucketRegionQueueJUnitTest.java
##########
@@ -199,4 +256,34 @@ GatewaySenderEventImpl createMockGatewaySenderEvent(Object key, TransactionId tI
when(event.getKey()).thenReturn(key);
return event;
}
+
+ private GatewaySenderEventImpl createGatewaySenderEvent(LocalRegion lr, Operation operation,
+ Object key, Object value, long threadId, long sequenceId)
+ throws Exception {
+ when(lr.getKeyInfo(key, value, null)).thenReturn(new KeyInfo(key, null, null));
+ when(lr.getTXId()).thenReturn(null);
+
+
+
+ EntryEventImpl eei = EntryEventImpl.create(lr, operation, key, value, null, false, null);
+ eei.setEventId(new EventID(new byte[16], threadId, sequenceId));
+ GatewaySenderEventImpl gsei =
+ new GatewaySenderEventImpl(getEnumListenerEvent(operation), eei, null, true, false);
+ return gsei;
Review comment:
This can be simplified to
```
return new GatewaySenderEventImpl(getEnumListenerEvent(operation), eei, null, true, false);
```
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -668,4 +679,27 @@ public boolean isReadyForPeek() {
&& !this.eventSeqNumDeque.isEmpty() && getBucketAdvisor().isPrimary();
}
+ List<Object> getQueueList() {
Review comment:
This method should probably be marked as `@VisibleForTesting` and maybe renamed to make it clear that it's a test helped method.
--
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] DonalEvans commented on a change in pull request #6474: GEODE-9272: postpone marking of event as possible duplicate to the mo…
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6474:
URL: https://github.com/apache/geode/pull/6474#discussion_r639222918
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
##########
@@ -203,8 +206,7 @@ private void destroyFailedBatchRemovalMessageKeys() {
@Override
public void beforeAcquiringPrimaryState() {
- Iterator<Object> itr = eventSeqNumDeque.iterator();
- markEventsAsDuplicate(itr);
Review comment:
When I search the codebase for uses of this method, I don't find any. Are you sure it's still used?
--
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