[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6305: feature/GEODE-7674: Clear on PR with lucene index should throw exception

[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