[
https://issues.apache.org/jira/browse/GEODE-9016?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17298293#comment-17298293
]
ASF GitHub Bot commented on GEODE-9016:
---------------------------------------
kirklund commented on a change in pull request #6104:
URL: https://github.com/apache/geode/pull/6104#discussion_r590657219
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/DistributedPutAllOperation.java
##########
@@ -814,6 +818,46 @@ protected FilterRoutingInfo getRecipientFilterRouting(Set
cacheOpRecipients) {
return consolidated;
}
+ @Override
+ protected void removeDestroyTokensFromCqResultKeys(FilterRoutingInfo
filterRouting) {
+ for (InternalDistributedMember m : filterRouting.getMembers()) {
+ FilterInfo filterInfo = filterRouting.getFilterInfo(m);
+ if (filterInfo.getCQs() == null) {
+ continue;
+ }
+
+ CacheDistributionAdvisor.CacheProfile cf =
+ (CacheDistributionAdvisor.CacheProfile) ((Bucket)
getRegion()).getPartitionedRegion()
+ .getCacheDistributionAdvisor().getProfile(m);
+
+ if (cf == null || cf.filterProfile == null ||
cf.filterProfile.isLocalProfile()
+ || cf.filterProfile.getCqMap().isEmpty()) {
+ continue;
+ }
+
+ for (Object value : cf.filterProfile.getCqMap().values()) {
Review comment:
In the future, you might want to try extracting some of those
inner-loops or inner-if-blocks to their own methods. How you name those methods
can then really improve understanding. It's also too easy to get lost in those
deeply nested structures like this.
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/cache/query/cq/dunit/PartitionedRegionCqQueryDUnitTest.java
##########
@@ -120,6 +123,60 @@
private static int bridgeServerPort;
+ @Test
+ public void testPutAllWithCQLocalDestroy() throws Exception {
+ VM server1 = getVM(0);
+ VM server2 = getVM(1);
+ VM client = getVM(2);
+
+ final String cqName = "testPutAllWithCQLocalDestroy_0";
+ createServer(server1);
+ createServer(server2);
+ final String host = Host.getHost(0).getHostName();
+ final int port = server2.invoke(() ->
PartitionedRegionCqQueryDUnitTest.getCacheServerPort());
+ createClient(client, port, host);
+ createCQ(client, cqName, cqs[0]);
+
+ int numObjects = 1000;
+
+ server1.invoke(() -> {
+ Region region = getCache().getRegion(SEPARATOR + "root" + SEPARATOR +
regions[0]);
+ Map buffer = new HashMap();
+ for (int i = 1; i < numObjects; i++) {
+ Portfolio p = new Portfolio(i);
+ buffer.put("" + i, p);
+ }
+ region.putAll(buffer);
+ });
+
+ client.invoke(() -> {
+ QueryService cqService = getCache().getQueryService();
+ CqQuery cqQuery = cqService.getCq(cqName);
+ if (cqQuery == null) {
+ fail("Failed to get CQ " + cqName);
+ }
Review comment:
Please use assertions instead of if-fail patterns:
```
assertThat(cqQuery)
.withFailMessage("Failed to get CQ " + cqName)
.isNotNull();
```
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/cache/query/cq/dunit/PartitionedRegionCqQueryDUnitTest.java
##########
@@ -120,6 +123,60 @@
private static int bridgeServerPort;
+ @Test
+ public void testPutAllWithCQLocalDestroy() throws Exception {
+ VM server1 = getVM(0);
+ VM server2 = getVM(1);
+ VM client = getVM(2);
+
+ final String cqName = "testPutAllWithCQLocalDestroy_0";
+ createServer(server1);
+ createServer(server2);
+ final String host = Host.getHost(0).getHostName();
+ final int port = server2.invoke(() ->
PartitionedRegionCqQueryDUnitTest.getCacheServerPort());
+ createClient(client, port, host);
+ createCQ(client, cqName, cqs[0]);
+
+ int numObjects = 1000;
+
+ server1.invoke(() -> {
+ Region region = getCache().getRegion(SEPARATOR + "root" + SEPARATOR +
regions[0]);
+ Map buffer = new HashMap();
Review comment:
You should avoid raw types in both product and test code. For example:
`Region<String, Portfolio>` or even `Region<String, Object>`. Same with Map.
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/cache/query/cq/dunit/PartitionedRegionCqQueryDUnitTest.java
##########
@@ -120,6 +123,60 @@
private static int bridgeServerPort;
+ @Test
+ public void testPutAllWithCQLocalDestroy() throws Exception {
+ VM server1 = getVM(0);
+ VM server2 = getVM(1);
+ VM client = getVM(2);
+
+ final String cqName = "testPutAllWithCQLocalDestroy_0";
+ createServer(server1);
+ createServer(server2);
+ final String host = Host.getHost(0).getHostName();
Review comment:
Avoid `Host` and just use:
```
String hostName = VM.getHostName();
```
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/cache/query/cq/dunit/PartitionedRegionCqQueryDUnitTest.java
##########
@@ -120,6 +123,60 @@
private static int bridgeServerPort;
+ @Test
+ public void testPutAllWithCQLocalDestroy() throws Exception {
+ VM server1 = getVM(0);
+ VM server2 = getVM(1);
+ VM client = getVM(2);
+
+ final String cqName = "testPutAllWithCQLocalDestroy_0";
+ createServer(server1);
+ createServer(server2);
+ final String host = Host.getHost(0).getHostName();
+ final int port = server2.invoke(() ->
PartitionedRegionCqQueryDUnitTest.getCacheServerPort());
+ createClient(client, port, host);
+ createCQ(client, cqName, cqs[0]);
+
+ int numObjects = 1000;
+
+ server1.invoke(() -> {
+ Region region = getCache().getRegion(SEPARATOR + "root" + SEPARATOR +
regions[0]);
+ Map buffer = new HashMap();
+ for (int i = 1; i < numObjects; i++) {
+ Portfolio p = new Portfolio(i);
+ buffer.put("" + i, p);
+ }
+ region.putAll(buffer);
+ });
+
+ client.invoke(() -> {
+ QueryService cqService = getCache().getQueryService();
+ CqQuery cqQuery = cqService.getCq(cqName);
+ if (cqQuery == null) {
+ fail("Failed to get CQ " + cqName);
+ }
+ try {
+ cqQuery.executeWithInitialResults();
+ } catch (Exception ex) {
+ fail("Failed to execute CQ " + cqName, ex);
+ }
+ });
+
+ server1.invoke(() -> {
+ Region region = getCache().getRegion(SEPARATOR + "root" + SEPARATOR +
regions[0]);
+ Map buffer = new HashMap();
+ for (int i = 1; i < numObjects; i++) {
+ Portfolio p = new Portfolio(-1 * i);
+ buffer.put("" + i, p);
+ }
+ region.putAll(buffer);
+ });
+
+ cqHelper.closeClient(client);
+ cqHelper.closeServer(server2);
+ cqHelper.closeServer(server1);
Review comment:
It's better (and more industry standard for writing JUnit tests) if you
can figure out a way to move cleanup code at the end of the test method to a
`tearDown` method annotated with `@After`. For example, if you make
`cqHelper.closeClient` return without error if client is null or not running,
then you could hoist these local variables to fields and the test would look
more like this:
```
private VM client;
private VM server1;
private VM server2;
@Before
public void setUp() {
server1 = getVM(0);
server2 = getVM(1);
client = getVM(2);
}
@After
public void tearDown() {
cqHelper.closeClient(client);
cqHelper.closeServer(server2);
cqHelper.closeServer(server1);
}
```
And `cqHelper` close methods could have some early-outs like this:
```
public void closeClient(VM vm) {
if (vm == null) {
return;
}
// etc
}
```
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/cache/query/cq/dunit/PartitionedRegionCqQueryDUnitTest.java
##########
@@ -120,6 +123,60 @@
private static int bridgeServerPort;
+ @Test
+ public void testPutAllWithCQLocalDestroy() throws Exception {
+ VM server1 = getVM(0);
+ VM server2 = getVM(1);
+ VM client = getVM(2);
+
+ final String cqName = "testPutAllWithCQLocalDestroy_0";
+ createServer(server1);
+ createServer(server2);
+ final String host = Host.getHost(0).getHostName();
+ final int port = server2.invoke(() ->
PartitionedRegionCqQueryDUnitTest.getCacheServerPort());
+ createClient(client, port, host);
+ createCQ(client, cqName, cqs[0]);
+
+ int numObjects = 1000;
+
+ server1.invoke(() -> {
+ Region region = getCache().getRegion(SEPARATOR + "root" + SEPARATOR +
regions[0]);
+ Map buffer = new HashMap();
+ for (int i = 1; i < numObjects; i++) {
+ Portfolio p = new Portfolio(i);
+ buffer.put("" + i, p);
+ }
+ region.putAll(buffer);
+ });
+
+ client.invoke(() -> {
+ QueryService cqService = getCache().getQueryService();
+ CqQuery cqQuery = cqService.getCq(cqName);
+ if (cqQuery == null) {
+ fail("Failed to get CQ " + cqName);
+ }
+ try {
+ cqQuery.executeWithInitialResults();
+ } catch (Exception ex) {
+ fail("Failed to execute CQ " + cqName, ex);
+ }
Review comment:
You don't need to catch Exception and invoke fail. It's better to let
`VM.invoke(...)` throw the Exception wrapped in an RMIException. You already
have `throws Exception` on the test method so JUnit will handle it and display
the full stack perfectly:
```
cqQuery.executeWithInitialResults();
```
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/cache/query/cq/dunit/PartitionedRegionCqQueryDUnitTest.java
##########
@@ -120,6 +123,60 @@
private static int bridgeServerPort;
+ @Test
+ public void testPutAllWithCQLocalDestroy() throws Exception {
+ VM server1 = getVM(0);
+ VM server2 = getVM(1);
+ VM client = getVM(2);
+
+ final String cqName = "testPutAllWithCQLocalDestroy_0";
+ createServer(server1);
+ createServer(server2);
+ final String host = Host.getHost(0).getHostName();
+ final int port = server2.invoke(() ->
PartitionedRegionCqQueryDUnitTest.getCacheServerPort());
Review comment:
Remove the name of the test anytime you see this, so you just have:
```
int port = server2.invoke(() -> getCacheServerPort());
```
----------------------------------------------------------------
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:
us...@infra.apache.org
> NullPointerException during PutAll with CQ LOCAL_DESTROY event
> --------------------------------------------------------------
>
> Key: GEODE-9016
> URL: https://issues.apache.org/jira/browse/GEODE-9016
> Project: Geode
> Issue Type: Bug
> Reporter: Jianxia Chen
> Assignee: Jianxia Chen
> Priority: Major
> Labels: pull-request-available
>
> It is possible that PutAll operation hits a NPE when CQ LOCAL_DESTROY event
> is generated.
> {code:java}
> java.lang.NullPointerException
> at
> java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:936)
> at
> org.apache.geode.cache.query.cq.internal.ServerCQResultsCachePartitionRegionImpl.remove(ServerCQResultsCachePartitionRegionImpl.java:69)
> at
> org.apache.geode.cache.query.cq.internal.ServerCQImpl.removeFromCqResultKeys(ServerCQImpl.java:297)
> at
> org.apache.geode.internal.cache.DistributedCacheOperation.removeDestroyTokensFromCqResultKeys(DistributedCacheOperation.java:743)
> at
> org.apache.geode.internal.cache.DistributedCacheOperation._distribute(DistributedCacheOperation.java:693)
> at
> org.apache.geode.internal.cache.DistributedCacheOperation.startOperation(DistributedCacheOperation.java:277)
> at
> org.apache.geode.internal.cache.DistributedRegion.postPutAllSend(DistributedRegion.java:3304)
> at
> org.apache.geode.internal.cache.LocalRegionDataView.postPutAll(LocalRegionDataView.java:358)
> at
> org.apache.geode.internal.cache.partitioned.PutAllPRMessage.doPostPutAll(PutAllPRMessage.java:568)
> at
> org.apache.geode.internal.cache.partitioned.PutAllPRMessage.doLocalPutAll(PutAllPRMessage.java:507)
> at
> org.apache.geode.internal.cache.partitioned.PutAllPRMessage.operateOnPartitionedRegion(PutAllPRMessage.java:326)
> at
> org.apache.geode.internal.cache.partitioned.PartitionMessage.process(PartitionMessage.java:333)
> at
> org.apache.geode.distributed.internal.DistributionMessage.scheduleAction(DistributionMessage.java:376)
> at
> org.apache.geode.distributed.internal.DistributionMessage$1.run(DistributionMessage.java:440)
> at
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
> at
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
> at
> org.apache.geode.distributed.internal.ClusterOperationExecutors.runUntilShutdown(ClusterOperationExecutors.java:442)
> at
> org.apache.geode.distributed.internal.ClusterOperationExecutors.doPartitionRegionThread(ClusterOperationExecutors.java:422)
> at
> org.apache.geode.logging.internal.executors.LoggingThreadFactory.lambda$newThread$0(LoggingThreadFactory.java:119)
> at java.lang.Thread.run(Thread.java:748)
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
