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 2020/11/12 21:58:13 UTC

[GitHub] [geode] bschuchardt opened a new pull request #5744: GEODE-8670: ReconnectDUnitTest is hiding a NullPointerException

bschuchardt opened a new pull request #5744:
URL: https://github.com/apache/geode/pull/5744


   1. Fixed all of the IgnorableException problems in ReconnectDUnitTest
   2. Modified InternalLocator to avoid throwing a NullPointerException and
      added a test for this change
   
   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] bschuchardt merged pull request #5744: GEODE-8670: ReconnectDUnitTest is hiding a NullPointerException

Posted by GitBox <gi...@apache.org>.
bschuchardt merged pull request #5744:
URL: https://github.com/apache/geode/pull/5744


   


----------------------------------------------------------------
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] bschuchardt commented on a change in pull request #5744: GEODE-8670: ReconnectDUnitTest is hiding a NullPointerException

Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5744:
URL: https://github.com/apache/geode/pull/5744#discussion_r523054006



##########
File path: geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorIntegrationTest.java
##########
@@ -137,6 +137,18 @@ public void startedLocatorHasLocator() throws IOException {
     assertThat(InternalLocator.hasLocator()).isTrue();
   }
 
+  @Test

Review comment:
       I think that, in fact, the test is invalid.  I'll post a better one.




----------------------------------------------------------------
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] bschuchardt commented on a change in pull request #5744: GEODE-8670: ReconnectDUnitTest is hiding a NullPointerException

Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5744:
URL: https://github.com/apache/geode/pull/5744#discussion_r523043949



##########
File path: geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorIntegrationTest.java
##########
@@ -137,6 +137,18 @@ public void startedLocatorHasLocator() throws IOException {
     assertThat(InternalLocator.hasLocator()).isTrue();
   }
 
+  @Test

Review comment:
       true, but I couldn't see how to write a better test.  Maybe you'd like to work with me on that?




----------------------------------------------------------------
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] bschuchardt commented on a change in pull request #5744: GEODE-8670: ReconnectDUnitTest is hiding a NullPointerException

Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5744:
URL: https://github.com/apache/geode/pull/5744#discussion_r523043949



##########
File path: geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorIntegrationTest.java
##########
@@ -137,6 +137,18 @@ public void startedLocatorHasLocator() throws IOException {
     assertThat(InternalLocator.hasLocator()).isTrue();
   }
 
+  @Test

Review comment:
       true, but I couldn't see how to write a better test.  Maybe you'd like to work with me on that?




----------------------------------------------------------------
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] echobravopapa commented on a change in pull request #5744: GEODE-8670: ReconnectDUnitTest is hiding a NullPointerException

Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #5744:
URL: https://github.com/apache/geode/pull/5744#discussion_r522459797



##########
File path: geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorIntegrationTest.java
##########
@@ -137,6 +137,18 @@ public void startedLocatorHasLocator() throws IOException {
     assertThat(InternalLocator.hasLocator()).isTrue();
   }
 
+  @Test

Review comment:
       I'm not saying this test is invalid, but it doesn't validate the fix below in `InternalLocator` i.e. if one comments out that code the test still passes...




----------------------------------------------------------------
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] Bill commented on a change in pull request #5744: GEODE-8670: ReconnectDUnitTest is hiding a NullPointerException

Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5744:
URL: https://github.com/apache/geode/pull/5744#discussion_r523284286



##########
File path: geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorIntegrationTest.java
##########
@@ -69,6 +70,8 @@
 
   @Before
   public void setUp() throws IOException {
+    // set a property to tell membership to create a new cluster
+    System.setProperty(GMSJoinLeave.BYPASS_DISCOVERY_PROPERTY, "true");

Review comment:
       I just noticed that @bschuchardt did add a `RestoreSystemProperties` rule so this is all good now




----------------------------------------------------------------
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] Bill commented on a change in pull request #5744: GEODE-8670: ReconnectDUnitTest is hiding a NullPointerException

Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5744:
URL: https://github.com/apache/geode/pull/5744#discussion_r523280748



##########
File path: geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorIntegrationTest.java
##########
@@ -97,16 +118,16 @@ public void constructs() {
 
     assertThatCode(() -> {
       internalLocator =
-          new InternalLocator(port, loggingSession, logFile, logWriter, securityLogWriter,
+          new InternalLocator(port, loggingSession, logFile, null, null,

Review comment:
       My read of `InternalLocator` is that it's ok to provide `null` log writers to the constructor ✓

##########
File path: geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorIntegrationTest.java
##########
@@ -69,6 +70,8 @@
 
   @Before
   public void setUp() throws IOException {
+    // set a property to tell membership to create a new cluster
+    System.setProperty(GMSJoinLeave.BYPASS_DISCOVERY_PROPERTY, "true");

Review comment:
       I assume this is done to speed up the test. I like it!
   
   But shouldn't we be restoring the system property after the test runs? I believe we have a JUnit rule for that?

##########
File path: geode-core/src/distributedTest/java/org/apache/geode/cache30/ReconnectDUnitTest.java
##########
@@ -1115,6 +1118,9 @@ public String description() {
    */
   @Test
   public void testReconnectFailsDueToBadCacheXML() throws Exception {
+    IgnoredException.addIgnoredException(DistributedSystemDisconnectedException.class);
+    IgnoredException.addIgnoredException("Cause parsing to fail");
+    IgnoredException.addIgnoredException("Exception while initializing an instance");

Review comment:
       Now that the (broken) regexp is no longer hiding exceptions, and you've explicitly added expectations for all of them, this test serves a good documentation for these exceptions and their role in reconnect.

##########
File path: geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorIntegrationTest.java
##########
@@ -88,6 +91,24 @@ public void tearDown() {
     }
   }
 
+  @Test
+  public void startedLocatorDoesNotAttemptReconnect() throws IOException, InterruptedException {
+    // start a locator that's not part of a cluster
+    final boolean joinCluster = false;
+    internalLocator = InternalLocator.startLocator(port, logFile, null,
+        null, bindAddress, joinCluster,
+        distributedSystemProperties, hostnameForClients, workingDirectory);
+    port = internalLocator.getPort();
+    // the locator shouldn't attempt a reconnect because it's not part of a cluster
+    internalLocator.stoppedForReconnect = true;
+    assertThat(internalLocator.attemptReconnect()).isFalse();
+    String output = FileUtils.readFileToString(logFile, Charset.defaultCharset());
+    assertThat(output).isNotEmpty();
+    assertThat(output).contains(InternalLocator.IGNORING_RECONNECT_REQUEST);
+  }

Review comment:
       I guess we had the reconnect cases covered (in the DUnit test) and this new test is for the no-reconnect case ✓




----------------------------------------------------------------
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