[
https://issues.apache.org/jira/browse/GEODE-8073?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17100307#comment-17100307
]
ASF GitHub Bot commented on GEODE-8073:
---------------------------------------
DonalEvans commented on a change in pull request #5055:
URL: https://github.com/apache/geode/pull/5055#discussion_r420457715
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -4593,39 +4593,58 @@ void updateNodeToBucketMap(
buckets = bucketKeys.keySet();
}
- for (Integer bucket : buckets) {
- Set keys = null;
- if (bucketKeys == null) {
- try {
- FetchKeysResponse fkr = FetchKeysMessage.send(member, this,
bucket, true);
- keys = fkr.waitForKeys();
- } catch (ForceReattemptException ignore) {
- failures.add(bucket);
- }
- } else {
- keys = bucketKeys.get(bucket);
+ fetchKeysAndValues(values, servConn, failures, member, bucketKeys,
buckets);
+ }
+ return failures;
+ }
+
+ void fetchKeysAndValues(VersionedObjectList values, ServerConnection
servConn,
+ Set<Integer> failures, InternalDistributedMember member,
+ HashMap<Integer, HashSet> bucketKeys, Set<Integer> buckets)
+ throws IOException {
+ for (Integer bucket : buckets) {
+ Set keys = null;
+ if (bucketKeys == null) {
+ try {
+ FetchKeysResponse fetchKeysResponse = getFetchKeysResponse(member,
bucket);
+ keys = fetchKeysResponse.waitForKeys();
+ } catch (ForceReattemptException ignore) {
Review comment:
Ah, I see. We may catch the exception, but we don't actually do anything
with it, even though we do _something_ if we catch it. This name is fine as it
is.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionTest.java
##########
@@ -573,6 +578,50 @@ public void
transactionThrowsTransactionDataRebalancedExceptionIfIsAForceReattem
.hasMessage(PartitionedRegion.DATA_MOVED_BY_REBALANCE).hasCause(exception);
}
+ @Test
+ public void failuresSavedIfFetchKeysThrows() throws Exception {
+ PartitionedRegion spyPartitionedRegion = spy(partitionedRegion);
+
+ VersionedObjectList values = mock(VersionedObjectList.class);
+ ServerConnection serverConnection = mock(ServerConnection.class);
+ Set<Integer> failures = mock(Set.class);
Review comment:
Instead of verifying the method call on the mocked set, you can verify
the contents of the real set, which should contain one item, which is the
`Integer` 1.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionTest.java
##########
@@ -573,6 +578,50 @@ public void
transactionThrowsTransactionDataRebalancedExceptionIfIsAForceReattem
.hasMessage(PartitionedRegion.DATA_MOVED_BY_REBALANCE).hasCause(exception);
}
+ @Test
+ public void failuresSavedIfFetchKeysThrows() throws Exception {
+ PartitionedRegion spyPartitionedRegion = spy(partitionedRegion);
+
+ VersionedObjectList values = mock(VersionedObjectList.class);
+ ServerConnection serverConnection = mock(ServerConnection.class);
+ Set<Integer> failures = mock(Set.class);
+ InternalDistributedMember member = mock(InternalDistributedMember.class);
+ Set<Integer> buckets = new HashSet<>();
+ buckets.add(1);
+ doThrow(new
ForceReattemptException("")).when(spyPartitionedRegion).getFetchKeysResponse(member,
+ 1);
+
+ spyPartitionedRegion.fetchKeysAndValues(values, serverConnection,
failures, member, null,
+ buckets);
+
+ verify(failures).add(1);
+ verify(spyPartitionedRegion, never()).getValuesForKeys(values,
serverConnection, null);
+ }
+
+ @Test
+ public void fetchKeysAndValuesInvokesGetValuesForKeys() throws Exception {
+ PartitionedRegion spyPartitionedRegion = spy(partitionedRegion);
+
+ VersionedObjectList values = mock(VersionedObjectList.class);
+ ServerConnection serverConnection = mock(ServerConnection.class);
+ Set<Integer> failures = mock(Set.class);
Review comment:
It's part of the "don't mock what you don't own" principle in TDD,
explained
[here](https://github.com/testdouble/contributing-tests/wiki/Don't-mock-what-you-don't-own)
and [here](https://blog.codecentric.de/en/2018/03/mock-what-when-how/). Also,
as discussed
[here](https://github.com/mockito/mockito/wiki/How-to-write-good-tests), if you
can avoid mocking a class, as in this case, then that's probably a good thing.
----------------------------------------------------------------
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 thrown in PartitionedRegion.handleOldNodes
> ---------------------------------------------------------------
>
> Key: GEODE-8073
> URL: https://issues.apache.org/jira/browse/GEODE-8073
> Project: Geode
> Issue Type: Bug
> Components: regions
> Reporter: Eric Shu
> Assignee: Eric Shu
> Priority: Major
> Labels: caching-applications
>
> The NPE can be thrown when a remote node is gone unexpectedly.
> {noformat}
> Caused by: java.lang.NullPointerException
> at
> org.apache.geode.internal.cache.PartitionedRegion.handleOldNodes(PartitionedRegion.java:4610)
> at
> org.apache.geode.internal.cache.PartitionedRegion.fetchEntries(PartitionedRegion.java:4689)
> at
> org.apache.geode.internal.cache.tier.sockets.BaseCommand.handleKVKeysPR(BaseCommand.java:1191)
> at
> org.apache.geode.internal.cache.tier.sockets.BaseCommand.handleKVAllKeys(BaseCommand.java:1124)
> at
> org.apache.geode.internal.cache.tier.sockets.BaseCommand.handleKeysValuesPolicy(BaseCommand.java:973)
> at
> org.apache.geode.internal.cache.tier.sockets.BaseCommand.fillAndSendRegisterInterestResponseChunks(BaseCommand.java:905)
> at
> org.apache.geode.internal.cache.tier.sockets.command.RegisterInterest61.cmdExecute(RegisterInterest61.java:260)
> at
> org.apache.geode.internal.cache.tier.sockets.BaseCommand.execute(BaseCommand.java:183)
> at
> org.apache.geode.internal.cache.tier.sockets.ServerConnection.doNormalMessage(ServerConnection.java:848)
> at
> org.apache.geode.internal.cache.tier.sockets.OriginalServerConnection.doOneMessage(OriginalServerConnection.java:72)
> at
> org.apache.geode.internal.cache.tier.sockets.ServerConnection.run(ServerConnection.java:1212)
> at
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
> at
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
> at
> org.apache.geode.internal.cache.tier.sockets.AcceptorImpl.lambda$initializeServerConnectionThreadPool$3(AcceptorImpl.java:686)
> at
> org.apache.geode.logging.internal.executors.LoggingThreadFactory.lambda$newThread$0(LoggingThreadFactory.java:119)
> at java.lang.Thread.run(Thread.java:748)
> {noformat}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
