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/05/20 21:43:51 UTC
[GitHub] [geode] dschneider-pivotal opened a new pull request #6503: GEODE-9295: prevent expiry message hangs
dschneider-pivotal opened a new pull request #6503:
URL: https://github.com/apache/geode/pull/6503
The process method on LatestLastAccessTimeMessage
will now send a reply of zero when it finds a null cache, region, or entry.
This will prevent the thread that sent the message from waiting forever for a reply.
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] dschneider-pivotal commented on pull request #6503: GEODE-9295: prevent expiry message hangs
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on pull request #6503:
URL: https://github.com/apache/geode/pull/6503#issuecomment-846233395
the dunit failure in SerialWANStatsDUnitTest is the known issue GEODE-9229
--
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 merged pull request #6503: GEODE-9295: prevent expiry message hangs
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal merged pull request #6503:
URL: https://github.com/apache/geode/pull/6503
--
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 #6503: GEODE-9295: prevent expiry message hangs
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6503:
URL: https://github.com/apache/geode/pull/6503#discussion_r637069833
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/LatestLastAccessTimeMessageTest.java
##########
@@ -14,30 +14,142 @@
*/
package org.apache.geode.internal.cache;
+import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
import java.util.Collections;
import java.util.Set;
import org.junit.Test;
import org.apache.geode.distributed.internal.ClusterDistributionManager;
-import org.apache.geode.distributed.internal.DistributionManager;
import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.InternalStatisticsDisabledException;
import org.apache.geode.internal.inet.LocalHostUtil;
public class LatestLastAccessTimeMessageTest {
@Test
- public void processMessageShouldLookForNullCache() throws Exception {
- final DistributionManager distributionManager = mock(DistributionManager.class);
+ public void processWithNullCacheRepliesZero() throws Exception {
final LatestLastAccessTimeReplyProcessor replyProcessor =
mock(LatestLastAccessTimeReplyProcessor.class);
final InternalDistributedRegion region = mock(InternalDistributedRegion.class);
Set<InternalDistributedMember> recipients = Collections.singleton(new InternalDistributedMember(
LocalHostUtil.getLocalHost(), 1234));
+ final LatestLastAccessTimeMessage<String> realLastAccessTimeMessage =
+ new LatestLastAccessTimeMessage<>(replyProcessor, recipients, region, "foo");
+ final LatestLastAccessTimeMessage<String> lastAccessTimeMessage =
+ spy(realLastAccessTimeMessage);
+ lastAccessTimeMessage.setSender(new InternalDistributedMember(
+ LocalHostUtil.getLocalHost(), 12345));
+ final ClusterDistributionManager dm = mock(ClusterDistributionManager.class);
+ when(dm.getCache()).thenReturn(null);
+
+ lastAccessTimeMessage.process(dm);
+
+ verify(lastAccessTimeMessage).sendReply(dm, 0);
+ }
+
+ @Test
+ public void processWithNullRegionRepliesZero() throws Exception {
+ final LatestLastAccessTimeReplyProcessor replyProcessor =
+ mock(LatestLastAccessTimeReplyProcessor.class);
+ final InternalDistributedRegion region = mock(InternalDistributedRegion.class);
+ Set<InternalDistributedMember> recipients = Collections.singleton(new InternalDistributedMember(
+ LocalHostUtil.getLocalHost(), 1234));
+ final LatestLastAccessTimeMessage<String> realLastAccessTimeMessage =
+ new LatestLastAccessTimeMessage<>(replyProcessor, recipients, region, "foo");
+ final LatestLastAccessTimeMessage<String> lastAccessTimeMessage =
+ spy(realLastAccessTimeMessage);
+ lastAccessTimeMessage.setSender(new InternalDistributedMember(
+ LocalHostUtil.getLocalHost(), 12345));
+ final ClusterDistributionManager dm = mock(ClusterDistributionManager.class);
+ final InternalCache cache = mock(InternalCache.class);
Review comment:
A lot of the repeated set-up in these test cases can be eliminated if you create a few fields and a `@Before` method like below. Also, you can use mock `InternalDistributedMember` instead of using real instances of the class to help keep this as much of a unit test as possible:
```
private InternalDistributedRegion region;
private ClusterDistributionManager dm;
private LatestLastAccessTimeMessage<String> lastAccessTimeMessage;
private InternalCache cache;
@Before
public void setUp() throws UnknownHostException {
final LatestLastAccessTimeReplyProcessor replyProcessor =
mock(LatestLastAccessTimeReplyProcessor.class);
region = mock(InternalDistributedRegion.class);
Set<InternalDistributedMember> recipients =
Collections.singleton(mock(InternalDistributedMember.class));
lastAccessTimeMessage =
spy(new LatestLastAccessTimeMessage<>(replyProcessor, recipients, region, "foo"));
lastAccessTimeMessage.setSender(mock(InternalDistributedMember.class));
dm = mock(ClusterDistributionManager.class);
cache = mock(InternalCache.class);
}
```
--
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 #6503: GEODE-9295: prevent expiry message hangs
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6503:
URL: https://github.com/apache/geode/pull/6503#discussion_r637097514
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/LatestLastAccessTimeMessageTest.java
##########
@@ -14,30 +14,142 @@
*/
package org.apache.geode.internal.cache;
+import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
import java.util.Collections;
import java.util.Set;
import org.junit.Test;
import org.apache.geode.distributed.internal.ClusterDistributionManager;
-import org.apache.geode.distributed.internal.DistributionManager;
import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.InternalStatisticsDisabledException;
import org.apache.geode.internal.inet.LocalHostUtil;
public class LatestLastAccessTimeMessageTest {
@Test
- public void processMessageShouldLookForNullCache() throws Exception {
- final DistributionManager distributionManager = mock(DistributionManager.class);
+ public void processWithNullCacheRepliesZero() throws Exception {
final LatestLastAccessTimeReplyProcessor replyProcessor =
mock(LatestLastAccessTimeReplyProcessor.class);
final InternalDistributedRegion region = mock(InternalDistributedRegion.class);
Set<InternalDistributedMember> recipients = Collections.singleton(new InternalDistributedMember(
LocalHostUtil.getLocalHost(), 1234));
+ final LatestLastAccessTimeMessage<String> realLastAccessTimeMessage =
+ new LatestLastAccessTimeMessage<>(replyProcessor, recipients, region, "foo");
+ final LatestLastAccessTimeMessage<String> lastAccessTimeMessage =
+ spy(realLastAccessTimeMessage);
+ lastAccessTimeMessage.setSender(new InternalDistributedMember(
+ LocalHostUtil.getLocalHost(), 12345));
+ final ClusterDistributionManager dm = mock(ClusterDistributionManager.class);
+ when(dm.getCache()).thenReturn(null);
+
+ lastAccessTimeMessage.process(dm);
+
+ verify(lastAccessTimeMessage).sendReply(dm, 0);
+ }
+
+ @Test
+ public void processWithNullRegionRepliesZero() throws Exception {
+ final LatestLastAccessTimeReplyProcessor replyProcessor =
+ mock(LatestLastAccessTimeReplyProcessor.class);
+ final InternalDistributedRegion region = mock(InternalDistributedRegion.class);
+ Set<InternalDistributedMember> recipients = Collections.singleton(new InternalDistributedMember(
+ LocalHostUtil.getLocalHost(), 1234));
+ final LatestLastAccessTimeMessage<String> realLastAccessTimeMessage =
+ new LatestLastAccessTimeMessage<>(replyProcessor, recipients, region, "foo");
+ final LatestLastAccessTimeMessage<String> lastAccessTimeMessage =
+ spy(realLastAccessTimeMessage);
+ lastAccessTimeMessage.setSender(new InternalDistributedMember(
+ LocalHostUtil.getLocalHost(), 12345));
+ final ClusterDistributionManager dm = mock(ClusterDistributionManager.class);
+ final InternalCache cache = mock(InternalCache.class);
Review comment:
Thanks for the feedback Donal. I have updates the PR with your suggested test cleanup.
--
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 pull request #6503: GEODE-9295: prevent expiry message hangs
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on pull request #6503:
URL: https://github.com/apache/geode/pull/6503#issuecomment-845583395
the dunit failure is a known issue: GEODE-9242
--
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