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/04/10 02:35:43 UTC
[GitHub] [geode] gesterzhou opened a new pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
gesterzhou opened a new pull request #6305:
URL: https://github.com/apache/geode/pull/6305
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] DonalEvans commented on a change in pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6305:
URL: https://github.com/apache/geode/pull/6305#discussion_r611883738
##########
File path: geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java
##########
@@ -281,6 +281,12 @@ public AsyncEventQueueFactory setForwardExpirationDestroy(boolean forward) {
return this;
}
+ // keep this method internal
+ public AsyncEventQueueFactory setPartitionedRegionClearUnsupported(boolean supported) {
+ gatewaySenderAttributes.partitionedRegionClearUnsupported = supported;
Review comment:
The method argument and field names here are confusing, as the field name is tracking whether PR clear *is not* supported, but the method argument name implies that it is setting whether PR clear *is* supported.
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionClearTest.java
##########
@@ -56,15 +57,19 @@
public class PartitionedRegionClearTest {
+ private GemFireCacheImpl cache;
+ private HashSet<AsyncEventQueue> allAEQs = new HashSet<>();
+ private PartitionedRegionClear partitionedRegionClear;
private DistributionManager distributionManager;
private PartitionedRegion partitionedRegion;
private RegionAdvisor regionAdvisor;
private InternalDistributedMember internalDistributedMember;
- private PartitionedRegionClear partitionedRegionClear;
-
@Before
public void setUp() {
+
+ cache = mock(GemFireCacheImpl.class);
+ partitionedRegion = mock(PartitionedRegion.class);
Review comment:
`partitionedRegion` is already defined on line 75, so this is redundant.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/AbstractGatewaySender.java
##########
@@ -143,6 +143,12 @@
protected boolean forwardExpirationDestroy;
+ /**
+ * An attribute to specify if Partitioned region clear operation is unsupported.
+ * Default is false, i.e.
Review comment:
This comment seems unfinished.
--
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] gesterzhou closed pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
Posted by GitBox <gi...@apache.org>.
gesterzhou closed pull request #6305:
URL: https://github.com/apache/geode/pull/6305
--
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] dschneider-pivotal commented on a change in pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6305:
URL: https://github.com/apache/geode/pull/6305#discussion_r612022529
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -396,6 +398,15 @@ void doClear(RegionEventImpl regionEvent, boolean cacheWrite) {
// Force all primary buckets to be created before clear.
assignAllPrimaryBuckets();
+ for (AsyncEventQueue asyncEventQueue : partitionedRegion.getCache()
+ .getAsyncEventQueues(false)) {
+ if (((AsyncEventQueueImpl) asyncEventQueue).isPartitionedRegionClearUnsupported()) {
+ throw new UnsupportedOperationException(
+ "PartitionedRegion clear is not supported on region "
Review comment:
I would remove "PartitionedRegion" from this message since that is more of an implementation detail. For the user they just need to know the op "clear" the region (and you added the name) and the reason (lucene index).
--
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] gesterzhou commented on pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on pull request #6305:
URL: https://github.com/apache/geode/pull/6305#issuecomment-818959247
after merged, it's better to create a new feature branch and pull request.
--
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] gesterzhou commented on a change in pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #6305:
URL: https://github.com/apache/geode/pull/6305#discussion_r611944421
##########
File path: geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java
##########
@@ -281,6 +281,12 @@ public AsyncEventQueueFactory setForwardExpirationDestroy(boolean forward) {
return this;
}
+ // keep this method internal
+ public AsyncEventQueueFactory setPartitionedRegionClearUnsupported(boolean supported) {
+ gatewaySenderAttributes.partitionedRegionClearUnsupported = supported;
Review comment:
fixed
--
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] gesterzhou commented on a change in pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #6305:
URL: https://github.com/apache/geode/pull/6305#discussion_r611944307
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/AbstractGatewaySender.java
##########
@@ -143,6 +143,12 @@
protected boolean forwardExpirationDestroy;
+ /**
+ * An attribute to specify if Partitioned region clear operation is unsupported.
+ * Default is false, i.e.
Review comment:
fixed
--
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] gesterzhou commented on a change in pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #6305:
URL: https://github.com/apache/geode/pull/6305#discussion_r611868108
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -396,6 +399,17 @@ void doClear(RegionEventImpl regionEvent, boolean cacheWrite) {
// Force all primary buckets to be created before clear.
assignAllPrimaryBuckets();
+ Iterator<AsyncEventQueue> allAEQsIterator =
+ partitionedRegion.getCache().getAsyncEventQueues(false).iterator();
+ while (allAEQsIterator.hasNext()) {
+ AsyncEventQueueImpl aeq = (AsyncEventQueueImpl) allAEQsIterator.next();
+ if (aeq.isPartitionedRegionClearSupported()) {
Review comment:
done
--
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] gesterzhou commented on a change in pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #6305:
URL: https://github.com/apache/geode/pull/6305#discussion_r611944149
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionClearTest.java
##########
@@ -56,15 +57,19 @@
public class PartitionedRegionClearTest {
+ private GemFireCacheImpl cache;
+ private HashSet<AsyncEventQueue> allAEQs = new HashSet<>();
+ private PartitionedRegionClear partitionedRegionClear;
private DistributionManager distributionManager;
private PartitionedRegion partitionedRegion;
private RegionAdvisor regionAdvisor;
private InternalDistributedMember internalDistributedMember;
- private PartitionedRegionClear partitionedRegionClear;
-
@Before
public void setUp() {
+
+ cache = mock(GemFireCacheImpl.class);
+ partitionedRegion = mock(PartitionedRegion.class);
Review comment:
caused by merge. kirk added today.
--
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] dschneider-pivotal commented on a change in pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6305:
URL: https://github.com/apache/geode/pull/6305#discussion_r611782166
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -396,6 +399,17 @@ void doClear(RegionEventImpl regionEvent, boolean cacheWrite) {
// Force all primary buckets to be created before clear.
assignAllPrimaryBuckets();
+ Iterator<AsyncEventQueue> allAEQsIterator =
+ partitionedRegion.getCache().getAsyncEventQueues(false).iterator();
+ while (allAEQsIterator.hasNext()) {
Review comment:
don't use an Iterator here. Instead do for (AsyncEventQueue asyncEventQueue: partitionedRegion.getCache().getAsyncEventQueues(false)) and then cast "asyncEventQueue" to an the Impl to call the internal method
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -396,6 +399,17 @@ void doClear(RegionEventImpl regionEvent, boolean cacheWrite) {
// Force all primary buckets to be created before clear.
assignAllPrimaryBuckets();
+ Iterator<AsyncEventQueue> allAEQsIterator =
+ partitionedRegion.getCache().getAsyncEventQueues(false).iterator();
+ while (allAEQsIterator.hasNext()) {
+ AsyncEventQueueImpl aeq = (AsyncEventQueueImpl) allAEQsIterator.next();
+ if (aeq.isPartitionedRegionClearSupported()) {
+ // this is a lucene aeq
+ throw new UnsupportedOperationException(
+ "PartitionedRegion clear is not supported on region with lucene index");
Review comment:
add the name of the region after "region". Also change "with lucene index" to "because it has a lucene index".
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -396,6 +399,17 @@ void doClear(RegionEventImpl regionEvent, boolean cacheWrite) {
// Force all primary buckets to be created before clear.
assignAllPrimaryBuckets();
+ Iterator<AsyncEventQueue> allAEQsIterator =
+ partitionedRegion.getCache().getAsyncEventQueues(false).iterator();
+ while (allAEQsIterator.hasNext()) {
+ AsyncEventQueueImpl aeq = (AsyncEventQueueImpl) allAEQsIterator.next();
+ if (aeq.isPartitionedRegionClearSupported()) {
+ // this is a lucene aeq
Review comment:
put this new block of code in a its own private method named something like "CheckForPartitionedRegionClearSupport" and get rid of this little one line comment.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -396,6 +399,17 @@ void doClear(RegionEventImpl regionEvent, boolean cacheWrite) {
// Force all primary buckets to be created before clear.
assignAllPrimaryBuckets();
+ Iterator<AsyncEventQueue> allAEQsIterator =
+ partitionedRegion.getCache().getAsyncEventQueues(false).iterator();
+ while (allAEQsIterator.hasNext()) {
+ AsyncEventQueueImpl aeq = (AsyncEventQueueImpl) allAEQsIterator.next();
+ if (aeq.isPartitionedRegionClearSupported()) {
Review comment:
the flag is confusing. As it is currently named "true" should mean that a PR region support clear. But the way you have it coded now "true" means we will throw an Unsupported exception. So do you want to change the property named to "PartitionedRegionClearUnsupported"?
Add some javadocs defining this property. If you set it to true then doing clear on a pr with that will throw the exception.
If you set to false then a clear on the pr will be allowed but will be ignored by the aeq/gateway.
--
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