[GitHub] [geode] jinmeiliao commented on a change in pull request #5567: GEODE-8547: Added impacts to show missing disk-stores

[GitBox <gi...@apache.org>]

jinmeiliao commented on a change in pull request #5567:
URL: https://github.com/apache/geode/pull/5567#discussion_r516987265



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoreCommand.java
##########
@@ -53,16 +53,19 @@ public ResultModel showMissingDiskStore() {
     Set<DistributedMember> dataMembers =
         DiskStoreCommandsUtils.getNormalMembers((InternalCache) getCache());
 
-    if (dataMembers.isEmpty()) {
-      return ResultModel.createInfo(CliStrings.NO_CACHING_MEMBERS_FOUND_MESSAGE);
+    List<ColocatedRegionDetails> missingRegions = null;
+    boolean cacheMemberExist = false;
+
+    if (!dataMembers.isEmpty()) {
+      cacheMemberExist = true;

Review comment:
       I believe you don't need this boolean. If no servers, `missingRegions` will be null, otherwise, it's empty or has data

##########
File path: geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoreCommandDUnitTest.java
##########
@@ -156,6 +160,71 @@ public void stoppingAndRestartingMemberDoesNotResultInMissingDiskStore() {
     assertThat(missingDiskStoreIds).isNull();
   }
 
+
+  @Test
+  public void stopAllMembersAndStart2ndLocator() throws Exception {
+    IgnoredException.addIgnoredException(DistributedSystemDisconnectedException.class);
+
+    MemberVM locator1 = lsRule.startLocatorVM(1, locator.getPort());
+
+    MemberVM server1 = lsRule.startServerVM(2, locator.getPort(), locator1.getPort());
+    @SuppressWarnings("unused")

Review comment:
       instead of suppress the warning, just don't assign it to a variable

##########
File path: geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoreCommandDUnitTest.java
##########
@@ -156,6 +160,71 @@ public void stoppingAndRestartingMemberDoesNotResultInMissingDiskStore() {
     assertThat(missingDiskStoreIds).isNull();
   }
 
+
+  @Test
+  public void stopAllMembersAndStart2ndLocator() throws Exception {
+    IgnoredException.addIgnoredException(DistributedSystemDisconnectedException.class);
+
+    MemberVM locator1 = lsRule.startLocatorVM(1, locator.getPort());
+
+    MemberVM server1 = lsRule.startServerVM(2, locator.getPort(), locator1.getPort());
+    @SuppressWarnings("unused")
+    MemberVM server2 = lsRule.startServerVM(3, locator.getPort(), locator1.getPort());
+
+    final String testRegionName = "regionA";
+    CommandStringBuilder csb;
+    csb = new CommandStringBuilder(CliStrings.CREATE_DISK_STORE)
+        .addOption(CliStrings.CREATE_DISK_STORE__NAME, "diskStore")
+        .addOption(CliStrings.CREATE_DISK_STORE__DIRECTORY_AND_SIZE, "diskStoreDir");
+    gfshConnector.executeAndAssertThat(csb.getCommandString()).statusIsSuccess();
+
+    CommandStringBuilder createRegion = new CommandStringBuilder(CliStrings.CREATE_REGION)
+        .addOption(CliStrings.CREATE_REGION__REGION, testRegionName)
+        .addOption(CliStrings.CREATE_REGION__DISKSTORE, "diskStore")
+        .addOption(CliStrings.CREATE_REGION__REGIONSHORTCUT,
+            RegionShortcut.PARTITION_REDUNDANT_PERSISTENT.toString());
+    await().untilAsserted(() -> gfshConnector.executeAndAssertThat(createRegion.getCommandString())
+        .statusIsSuccess());
+
+
+    lsRule.stop(1, false);
+
+    // Add data to the region
+    addData(server1, testRegionName);

Review comment:
       you also don't need to add data and do rebalance

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoreCommand.java
##########
@@ -113,7 +116,9 @@ private ResultModel toMissingDiskStoresTabularResult(
     }
 
     TabularResultModel missingRegionsSection = result.addTable(MISSING_COLOCATED_REGIONS_SECTION);
-    if (hasMissingColocatedRegions) {
+    if (!cacheMemberExist) {

Review comment:
       here you can do something like:
   ```
   if (missingColocatedRegions == null) {
     // set the header as "no caching members"
   }
   else if (missingColocatedRegions.isEmpty()) {
   ....
   }
   else{
   ....
   }

##########
File path: geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoreCommandDUnitTest.java
##########
@@ -156,6 +160,71 @@ public void stoppingAndRestartingMemberDoesNotResultInMissingDiskStore() {
     assertThat(missingDiskStoreIds).isNull();
   }
 
+
+  @Test
+  public void stopAllMembersAndStart2ndLocator() throws Exception {
+    IgnoredException.addIgnoredException(DistributedSystemDisconnectedException.class);
+
+    MemberVM locator1 = lsRule.startLocatorVM(1, locator.getPort());
+
+    MemberVM server1 = lsRule.startServerVM(2, locator.getPort(), locator1.getPort());
+    @SuppressWarnings("unused")
+    MemberVM server2 = lsRule.startServerVM(3, locator.getPort(), locator1.getPort());
+
+    final String testRegionName = "regionA";

Review comment:
       I believe you can just create a region without the diskstore, the same problem would still manifest. Make the test simpler




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