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/09/29 09:24:13 UTC
[GitHub] [geode] mivanac opened a new pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
mivanac opened a new pull request #5567:
URL: https://github.com/apache/geode/pull/5567
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] mivanac merged pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by GitBox <gi...@apache.org>.
mivanac merged pull request #5567:
URL: https://github.com/apache/geode/pull/5567
----------------------------------------------------------------
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] mivanac commented on pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5567:
URL: https://github.com/apache/geode/pull/5567#issuecomment-720912624
Hi, when I remove my changes in ShowMissingDiskStoreCommand, this TC fails. Problem here is that without these changes, command is rejected, with message "No caching members found".
----------------------------------------------------------------
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] jinmeiliao commented on a change in pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by 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
[GitHub] [geode] mivanac commented on pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5567:
URL: https://github.com/apache/geode/pull/5567#issuecomment-721383598
We are solving a problem when all members are shutdown, and after some time we are trying to startup. Let say, that due to some problems locator with latest info, cannot start. In this case, remaining locator will not start, so we need to use command revoke missing-disk-store, which needs input from command show missing disk-store.
----------------------------------------------------------------
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] mivanac commented on pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5567:
URL: https://github.com/apache/geode/pull/5567#issuecomment-704730098
Test added.
----------------------------------------------------------------
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] jinmeiliao commented on a change in pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5567:
URL: https://github.com/apache/geode/pull/5567#discussion_r517420791
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoreCommand.java
##########
@@ -95,7 +96,8 @@ private ResultModel toMissingDiskStoresTabularResult(
ResultModel result = new ResultModel();
boolean hasMissingDiskStores = missingDiskStores.length != 0;
- boolean hasMissingColocatedRegions = !missingColocatedRegions.isEmpty();
+ boolean hasMissingColocatedRegions =
Review comment:
this variable is not used anymore.
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoreCommand.java
##########
@@ -95,7 +96,8 @@ private ResultModel toMissingDiskStoresTabularResult(
ResultModel result = new ResultModel();
boolean hasMissingDiskStores = missingDiskStores.length != 0;
Review comment:
actually you can get rid of this variable and inline this too, to make it symetric.
----------------------------------------------------------------
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] jinmeiliao commented on pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on pull request #5567:
URL: https://github.com/apache/geode/pull/5567#issuecomment-721378276
I believe it's as designed that if no members are up and running, then `show missing disk-store` will not work. I don't quite understand what's the problem you are trying to solve by this fix?
----------------------------------------------------------------
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] jinmeiliao commented on a change in pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5567:
URL: https://github.com/apache/geode/pull/5567#discussion_r516989089
##########
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. Maybe add a comment in your test making it clear that you are starting a stale locator first (locator 1 shutdown before locator0, and now you are starting locator 1 first), locator 1 won't start properly until a diskstore on locator 0 is revoked
----------------------------------------------------------------
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] mivanac commented on pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5567:
URL: https://github.com/apache/geode/pull/5567#issuecomment-708593672
@kirklund is added test OK with you.
----------------------------------------------------------------
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] mivanac commented on pull request #5567: GEODE-8547: Added impacts to show missing disk-stores
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5567:
URL: https://github.com/apache/geode/pull/5567#issuecomment-721699428
Thanks for the comments.
----------------------------------------------------------------
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