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/03/09 19:52:01 UTC
[GitHub] [geode] kirklund commented on a change in pull request #6104: GEODE-9016: Fix the NPE for PutAll with CQ LOCAL_DESTROY message type
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:
users@infra.apache.org