You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by YehEmily <gi...@git.apache.org> on 2017/08/04 19:07:00 UTC
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
GitHub user YehEmily opened a pull request:
https://github.com/apache/geode/pull/687
GEODE-3258: Refactoring DiskStoreCommands
[View the JIRA ticket here.](https://issues.apache.org/jira/browse/GEODE-3258)
`DiskStoreCommands` was a huge class that contained a large number of commands. These commands have been split into their own individual command classes. Two methods shared by several commands were moved into `DiskStoreCommandsUtils` (it has been mentioned before that --Utils classes should be avoided, but because a better home for these methods could not be found, this issue may be resolved later in the future).
Some tests were also updated - `DiskStoreCommandsJUnitTest` and `ListAndDescribeDiskStoreCommandsDUnitTest`. It really looks like the two tests share many of the same functionalities... I think that `ListAndDescribeDiskStoreCommandsDUnitTest` could be removed, since `DiskStoreCommandsJUnitTest` already seems to test `ListDiskStoreCommand` and `DescribeDiskStoreCommand`, and `DiskStoreCommandsJUnitTest` should be updated to test more commands, since it only seems to be testing `list disk-store` and `describe disk-store`.
**TESTING STATUS: Precheckin in progress**
- [ ] JIRA ticket referened
- [ ] PR rebased
- [ ] Initial commit is single and squashed
- [ ] `gradlew build` runs cleanly
- [ ] Unit tests updated
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/YehEmily/geode GEODE-3258
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/687.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #687
----
commit 55bd2b1613ea02a895b8f38b9760d14c5ffa531e
Author: YehEmily <em...@gmail.com>
Date: 2017-08-03T16:00:08Z
GEODE-3258: Refactoring DiskStoreCommands
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131777554
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CompactDiskStoreCommand.java ---
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.springframework.shell.core.annotation.CliCommand;
+import org.springframework.shell.core.annotation.CliOption;
+
+import org.apache.geode.cache.persistence.PersistentID;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.DistributedSystemMXBean;
+import org.apache.geode.management.ManagementService;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.ConverterHint;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.CliUtil;
+import org.apache.geode.management.internal.cli.LogWrapper;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.CompositeResultData;
+import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.messages.CompactRequest;
+import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.ResourcePermission;
+
+public class CompactDiskStoreCommand implements GfshCommand {
+ @CliCommand(value = CliStrings.COMPACT_DISK_STORE, help = CliStrings.COMPACT_DISK_STORE__HELP)
+ @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DISKSTORE})
+ @ResourceOperation(resource = ResourcePermission.Resource.CLUSTER,
+ operation = ResourcePermission.Operation.MANAGE, target = ResourcePermission.Target.DISK)
+ public Result compactDiskStore(
+ @CliOption(key = CliStrings.COMPACT_DISK_STORE__NAME, mandatory = true,
+ optionContext = ConverterHint.DISKSTORE,
+ help = CliStrings.COMPACT_DISK_STORE__NAME__HELP) String diskStoreName,
+ @CliOption(key = {CliStrings.GROUP, CliStrings.GROUPS},
+ help = CliStrings.COMPACT_DISK_STORE__GROUP__HELP) String[] groups) {
+ Result result;
+
+ try {
+ // disk store exists validation
+ if (!diskStoreExists(diskStoreName)) {
+ result = ResultBuilder.createUserErrorResult(
+ CliStrings.format(CliStrings.COMPACT_DISK_STORE__DISKSTORE_0_DOES_NOT_EXIST,
+ new Object[] {diskStoreName}));
+ } else {
+ InternalDistributedSystem ds = getCache().getInternalDistributedSystem();
+
+ Map<DistributedMember, PersistentID> overallCompactInfo = new HashMap<>();
+
+ Set<?> otherMembers = ds.getDistributionManager().getOtherNormalDistributionManagerIds();
+ Set<InternalDistributedMember> allMembers = new HashSet<>();
+
+ for (Object member : otherMembers) {
+ allMembers.add((InternalDistributedMember) member);
+ }
+ allMembers.add(ds.getDistributedMember());
+
+ String groupInfo = "";
+ // if groups are specified, find members in the specified group
+ if (groups != null && groups.length > 0) {
+ groupInfo = CliStrings.format(CliStrings.COMPACT_DISK_STORE__MSG__FOR_GROUP,
+ new Object[] {Arrays.toString(groups) + "."});
+ final Set<InternalDistributedMember> selectedMembers = new HashSet<>();
+ List<String> targetedGroups = Arrays.asList(groups);
+ for (InternalDistributedMember member : allMembers) {
+ List<String> memberGroups = member.getGroups();
+ if (!Collections.disjoint(targetedGroups, memberGroups)) {
+ selectedMembers.add(member);
+ }
+ }
+
+ allMembers = selectedMembers;
+ }
+
+ // allMembers should not be empty when groups are not specified - it'll
+ // have at least one member
+ if (allMembers.isEmpty()) {
+ result = ResultBuilder.createUserErrorResult(
+ CliStrings.format(CliStrings.COMPACT_DISK_STORE__NO_MEMBERS_FOUND_IN_SPECIFED_GROUP,
+ new Object[] {Arrays.toString(groups)}));
+ } else {
+ // first invoke on local member if it exists in the targeted set
+ if (allMembers.remove(ds.getDistributedMember())) {
+ PersistentID compactedDiskStoreId = CompactRequest.compactDiskStore(diskStoreName);
+ if (compactedDiskStoreId != null) {
+ overallCompactInfo.put(ds.getDistributedMember(), compactedDiskStoreId);
+ }
+ }
+
+ // was this local member the only one? Then don't try to send
+ // CompactRequest. Otherwise, send the request to others
+ if (!allMembers.isEmpty()) {
+ // Invoke compact on all 'other' members
+ Map<DistributedMember, PersistentID> memberCompactInfo =
+ CompactRequest.send(ds.getDistributionManager(), diskStoreName, allMembers);
+ if (memberCompactInfo != null && !memberCompactInfo.isEmpty()) {
+ overallCompactInfo.putAll(memberCompactInfo);
+ memberCompactInfo.clear();
+ }
+ String notExecutedMembers = CompactRequest.getNotExecutedMembers();
+ if (notExecutedMembers != null && !notExecutedMembers.isEmpty()) {
+ LogWrapper.getInstance()
+ .info("compact disk-store \"" + diskStoreName
+ + "\" message was scheduled to be sent to but was not send to "
--- End diff --
I realize this goes beyond a simple class-extraction refactor, this message is poorly worded. A couple alternatives:
"message was scheduled to be sent but was not for "
or
"message was scheduled to be sent to but was not sent to "
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131783193
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CompactOfflineDiskStoreCommand.java ---
@@ -0,0 +1,178 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.apache.geode.management.internal.cli.commands.DiskStoreCommandsUtils.configureLogging;
--- End diff --
Agreed - thank you for your feedback! I'll update the PR!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by YehEmily <gi...@git.apache.org>.
GitHub user YehEmily reopened a pull request:
https://github.com/apache/geode/pull/687
GEODE-3258: Refactoring DiskStoreCommands
[View the JIRA ticket here.](https://issues.apache.org/jira/browse/GEODE-3258)
`DiskStoreCommands` was a huge class that contained a large number of commands. These commands have been split into their own individual command classes. Two methods shared by several commands were moved into `DiskStoreCommandsUtils` (it has been mentioned before that --Utils classes should be avoided, but because a better home for these methods could not be found, this issue may be resolved later in the future).
Some tests were also updated - `DiskStoreCommandsJUnitTest` and `ListAndDescribeDiskStoreCommandsDUnitTest`. It really looks like the two tests share many of the same functionalities... I think that `ListAndDescribeDiskStoreCommandsDUnitTest` could be removed, since `DiskStoreCommandsJUnitTest` already seems to test `ListDiskStoreCommand` and `DescribeDiskStoreCommand`, and `DiskStoreCommandsJUnitTest` should be updated to test more commands, since it only seems to be testing `list disk-store` and `describe disk-store`.
**TESTING STATUS: Precheckin in progress**
- [x] JIRA ticket referened
- [x] PR rebased
- [x] Initial commit is single and squashed
- [x] `gradlew build` runs cleanly
- [x] Unit tests updated
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/YehEmily/geode GEODE-3258
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/687.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #687
----
commit 55bd2b1613ea02a895b8f38b9760d14c5ffa531e
Author: YehEmily <em...@gmail.com>
Date: 2017-08-03T16:00:08Z
GEODE-3258: Refactoring DiskStoreCommands
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode/pull/687
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131783008
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CompactDiskStoreCommand.java ---
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.springframework.shell.core.annotation.CliCommand;
+import org.springframework.shell.core.annotation.CliOption;
+
+import org.apache.geode.cache.persistence.PersistentID;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.DistributedSystemMXBean;
+import org.apache.geode.management.ManagementService;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.ConverterHint;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.CliUtil;
+import org.apache.geode.management.internal.cli.LogWrapper;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.CompositeResultData;
+import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.messages.CompactRequest;
+import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.ResourcePermission;
+
+public class CompactDiskStoreCommand implements GfshCommand {
+ @CliCommand(value = CliStrings.COMPACT_DISK_STORE, help = CliStrings.COMPACT_DISK_STORE__HELP)
+ @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DISKSTORE})
+ @ResourceOperation(resource = ResourcePermission.Resource.CLUSTER,
+ operation = ResourcePermission.Operation.MANAGE, target = ResourcePermission.Target.DISK)
+ public Result compactDiskStore(
+ @CliOption(key = CliStrings.COMPACT_DISK_STORE__NAME, mandatory = true,
+ optionContext = ConverterHint.DISKSTORE,
+ help = CliStrings.COMPACT_DISK_STORE__NAME__HELP) String diskStoreName,
+ @CliOption(key = {CliStrings.GROUP, CliStrings.GROUPS},
+ help = CliStrings.COMPACT_DISK_STORE__GROUP__HELP) String[] groups) {
+ Result result;
+
+ try {
+ // disk store exists validation
+ if (!diskStoreExists(diskStoreName)) {
+ result = ResultBuilder.createUserErrorResult(
+ CliStrings.format(CliStrings.COMPACT_DISK_STORE__DISKSTORE_0_DOES_NOT_EXIST,
+ new Object[] {diskStoreName}));
+ } else {
+ InternalDistributedSystem ds = getCache().getInternalDistributedSystem();
+
+ Map<DistributedMember, PersistentID> overallCompactInfo = new HashMap<>();
+
+ Set<?> otherMembers = ds.getDistributionManager().getOtherNormalDistributionManagerIds();
+ Set<InternalDistributedMember> allMembers = new HashSet<>();
+
+ for (Object member : otherMembers) {
+ allMembers.add((InternalDistributedMember) member);
+ }
+ allMembers.add(ds.getDistributedMember());
+
+ String groupInfo = "";
+ // if groups are specified, find members in the specified group
+ if (groups != null && groups.length > 0) {
+ groupInfo = CliStrings.format(CliStrings.COMPACT_DISK_STORE__MSG__FOR_GROUP,
+ new Object[] {Arrays.toString(groups) + "."});
+ final Set<InternalDistributedMember> selectedMembers = new HashSet<>();
+ List<String> targetedGroups = Arrays.asList(groups);
+ for (InternalDistributedMember member : allMembers) {
+ List<String> memberGroups = member.getGroups();
+ if (!Collections.disjoint(targetedGroups, memberGroups)) {
+ selectedMembers.add(member);
+ }
+ }
+
+ allMembers = selectedMembers;
+ }
+
+ // allMembers should not be empty when groups are not specified - it'll
+ // have at least one member
+ if (allMembers.isEmpty()) {
+ result = ResultBuilder.createUserErrorResult(
+ CliStrings.format(CliStrings.COMPACT_DISK_STORE__NO_MEMBERS_FOUND_IN_SPECIFED_GROUP,
+ new Object[] {Arrays.toString(groups)}));
+ } else {
+ // first invoke on local member if it exists in the targeted set
+ if (allMembers.remove(ds.getDistributedMember())) {
+ PersistentID compactedDiskStoreId = CompactRequest.compactDiskStore(diskStoreName);
+ if (compactedDiskStoreId != null) {
+ overallCompactInfo.put(ds.getDistributedMember(), compactedDiskStoreId);
+ }
+ }
+
+ // was this local member the only one? Then don't try to send
+ // CompactRequest. Otherwise, send the request to others
+ if (!allMembers.isEmpty()) {
+ // Invoke compact on all 'other' members
+ Map<DistributedMember, PersistentID> memberCompactInfo =
+ CompactRequest.send(ds.getDistributionManager(), diskStoreName, allMembers);
+ if (memberCompactInfo != null && !memberCompactInfo.isEmpty()) {
+ overallCompactInfo.putAll(memberCompactInfo);
+ memberCompactInfo.clear();
+ }
+ String notExecutedMembers = CompactRequest.getNotExecutedMembers();
+ if (notExecutedMembers != null && !notExecutedMembers.isEmpty()) {
+ LogWrapper.getInstance()
+ .info("compact disk-store \"" + diskStoreName
+ + "\" message was scheduled to be sent to but was not send to "
--- End diff --
I agree, the message should be improved! Thank you for your suggestions! I ended up going with `"message was scheduled to be sent to " + notExecutedMembers + ", but was not sent."`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily closed the pull request at:
https://github.com/apache/geode/pull/687
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131787137
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ValidateDiskStoreCommand.java ---
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.apache.geode.management.internal.cli.commands.DiskStoreCommandsUtils.configureLogging;
--- End diff --
Remove import. See my previous moment regarding utility method calls.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131782938
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CompactOfflineDiskStoreCommand.java ---
@@ -0,0 +1,178 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.apache.geode.management.internal.cli.commands.DiskStoreCommandsUtils.configureLogging;
--- End diff --
I would delete this import. Both of the DiskStoreCommandsUtils methods should be treated the same. Either provide static imports for both configureLogging and validatedDirectories, or for neither of them. I would opt for the later so it's obvious from the calls that these are static utility methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on the issue:
https://github.com/apache/geode/pull/687
I'll go ahead and review your command refactorings in this PR, keeping in mind that the test will be dealt with under the other JIRA. I agree with not changing those two DUnit tests at this time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131787338
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ValidateDiskStoreCommand.java ---
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.apache.geode.management.internal.cli.commands.DiskStoreCommandsUtils.configureLogging;
--- End diff --
Fixed! I also found one more occurrence in `ValidateDiskStoreCommand` and removed that one, too. Thank you!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on the issue:
https://github.com/apache/geode/pull/687
@pdxrunner The tests that cover these commands are part of a separate JIRA ticket ([GEODE-1359](https://issues.apache.org/jira/browse/GEODE-1359)), so @jinmeiliao recommended just refactoring the commands and leaving the tests to be refactored in the future. I think I will try to add tests to cover the rest of the commands to `DiskStoreCommandsJUnitTest`, since there are very few commands covered by that test, but I think I'll leave `DiskStoreCommandsDUnitTest` and `ListAndDescribeDiskStoreCommandsDUnitTest ` alone for now. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131787125
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListDiskStoreCommand.java ---
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.springframework.shell.core.annotation.CliCommand;
+
+import org.apache.geode.SystemFailure;
+import org.apache.geode.cache.execute.Execution;
+import org.apache.geode.cache.execute.FunctionInvocationTargetException;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.execute.AbstractExecution;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.CliUtil;
+import org.apache.geode.management.internal.cli.domain.DiskStoreDetails;
+import org.apache.geode.management.internal.cli.functions.ListDiskStoresFunction;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.cli.result.ResultDataException;
+import org.apache.geode.management.internal.cli.result.TabularResultData;
+import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.ResourcePermission;
+
+public class ListDiskStoreCommand implements GfshCommand {
+ @CliCommand(value = CliStrings.LIST_DISK_STORE, help = CliStrings.LIST_DISK_STORE__HELP)
+ @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DISKSTORE})
+ @ResourceOperation(resource = ResourcePermission.Resource.CLUSTER,
+ operation = ResourcePermission.Operation.READ)
+ public Result listDiskStore() {
+ try {
+ Set<DistributedMember> dataMembers = getNormalMembers(getCache());
+
+ if (dataMembers.isEmpty()) {
+ return ResultBuilder.createInfoResult(CliStrings.NO_CACHING_MEMBERS_FOUND_MESSAGE);
+ }
+
+ return toTabularResult(getDiskStoreListing(dataMembers));
+ } catch (FunctionInvocationTargetException ignore) {
+ return ResultBuilder.createGemFireErrorResult(CliStrings
+ .format(CliStrings.COULD_NOT_EXECUTE_COMMAND_TRY_AGAIN, CliStrings.LIST_DISK_STORE));
+ } catch (VirtualMachineError e) {
+ SystemFailure.initiateFailure(e);
+ throw e;
+ } catch (Throwable t) {
+ SystemFailure.checkFailure();
+ return ResultBuilder.createGemFireErrorResult(
+ String.format(CliStrings.LIST_DISK_STORE__ERROR_MESSAGE, toString(t, isDebugging())));
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ public List<DiskStoreDetails> getDiskStoreListing(Set<DistributedMember> members) {
+ final Execution membersFunctionExecutor = getMembersFunctionExecutor(members);
+ if (membersFunctionExecutor instanceof AbstractExecution) {
+ ((AbstractExecution) membersFunctionExecutor).setIgnoreDepartedMembers(true);
+ }
+
+ final ResultCollector<?, ?> resultCollector =
+ membersFunctionExecutor.execute(new ListDiskStoresFunction());
+
+ final List<?> results = (List<?>) resultCollector.getResult();
+ final List<DiskStoreDetails> distributedSystemMemberDiskStores =
+ new ArrayList<>(results.size());
+
+ for (final Object result : results) {
+ if (result instanceof Set) { // ignore FunctionInvocationTargetExceptions and other
+ // Exceptions...
+ distributedSystemMemberDiskStores.addAll((Set<DiskStoreDetails>) result);
+ }
+ }
+
+ Collections.sort(distributedSystemMemberDiskStores);
+
+ return distributedSystemMemberDiskStores;
+ }
+
+ private Result toTabularResult(final List<DiskStoreDetails> diskStoreList)
+ throws ResultDataException {
+ if (!diskStoreList.isEmpty()) {
+ final TabularResultData diskStoreData = ResultBuilder.createTabularResultData();
+
+ for (final DiskStoreDetails diskStoreDetails : diskStoreList) {
+ diskStoreData.accumulate("Member Name", diskStoreDetails.getMemberName());
+ diskStoreData.accumulate("Member Id", diskStoreDetails.getMemberId());
+ diskStoreData.accumulate("Disk Store Name", diskStoreDetails.getName());
+ diskStoreData.accumulate("Disk Store ID", diskStoreDetails.getId());
+ }
+
+ return ResultBuilder.buildResult(diskStoreData);
+ } else {
+ return ResultBuilder
+ .createInfoResult(CliStrings.LIST_DISK_STORE__DISK_STORES_NOT_FOUND_MESSAGE);
+ }
+ }
+
+ public Set<DistributedMember> getNormalMembers(final InternalCache cache) {
--- End diff --
I think I had some kind of reason for keeping this method out of the Utils class, but I can't remember it anymore... I made the fix and everything seems fine, though! Thank you for your feedback!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131785934
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListDiskStoreCommand.java ---
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.springframework.shell.core.annotation.CliCommand;
+
+import org.apache.geode.SystemFailure;
+import org.apache.geode.cache.execute.Execution;
+import org.apache.geode.cache.execute.FunctionInvocationTargetException;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.execute.AbstractExecution;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.CliUtil;
+import org.apache.geode.management.internal.cli.domain.DiskStoreDetails;
+import org.apache.geode.management.internal.cli.functions.ListDiskStoresFunction;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.cli.result.ResultDataException;
+import org.apache.geode.management.internal.cli.result.TabularResultData;
+import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.ResourcePermission;
+
+public class ListDiskStoreCommand implements GfshCommand {
+ @CliCommand(value = CliStrings.LIST_DISK_STORE, help = CliStrings.LIST_DISK_STORE__HELP)
+ @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DISKSTORE})
+ @ResourceOperation(resource = ResourcePermission.Resource.CLUSTER,
+ operation = ResourcePermission.Operation.READ)
+ public Result listDiskStore() {
+ try {
+ Set<DistributedMember> dataMembers = getNormalMembers(getCache());
+
+ if (dataMembers.isEmpty()) {
+ return ResultBuilder.createInfoResult(CliStrings.NO_CACHING_MEMBERS_FOUND_MESSAGE);
+ }
+
+ return toTabularResult(getDiskStoreListing(dataMembers));
+ } catch (FunctionInvocationTargetException ignore) {
+ return ResultBuilder.createGemFireErrorResult(CliStrings
+ .format(CliStrings.COULD_NOT_EXECUTE_COMMAND_TRY_AGAIN, CliStrings.LIST_DISK_STORE));
+ } catch (VirtualMachineError e) {
+ SystemFailure.initiateFailure(e);
+ throw e;
+ } catch (Throwable t) {
+ SystemFailure.checkFailure();
+ return ResultBuilder.createGemFireErrorResult(
+ String.format(CliStrings.LIST_DISK_STORE__ERROR_MESSAGE, toString(t, isDebugging())));
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ public List<DiskStoreDetails> getDiskStoreListing(Set<DistributedMember> members) {
+ final Execution membersFunctionExecutor = getMembersFunctionExecutor(members);
+ if (membersFunctionExecutor instanceof AbstractExecution) {
+ ((AbstractExecution) membersFunctionExecutor).setIgnoreDepartedMembers(true);
+ }
+
+ final ResultCollector<?, ?> resultCollector =
+ membersFunctionExecutor.execute(new ListDiskStoresFunction());
+
+ final List<?> results = (List<?>) resultCollector.getResult();
+ final List<DiskStoreDetails> distributedSystemMemberDiskStores =
+ new ArrayList<>(results.size());
+
+ for (final Object result : results) {
+ if (result instanceof Set) { // ignore FunctionInvocationTargetExceptions and other
+ // Exceptions...
+ distributedSystemMemberDiskStores.addAll((Set<DiskStoreDetails>) result);
+ }
+ }
+
+ Collections.sort(distributedSystemMemberDiskStores);
+
+ return distributedSystemMemberDiskStores;
+ }
+
+ private Result toTabularResult(final List<DiskStoreDetails> diskStoreList)
+ throws ResultDataException {
+ if (!diskStoreList.isEmpty()) {
+ final TabularResultData diskStoreData = ResultBuilder.createTabularResultData();
+
+ for (final DiskStoreDetails diskStoreDetails : diskStoreList) {
+ diskStoreData.accumulate("Member Name", diskStoreDetails.getMemberName());
+ diskStoreData.accumulate("Member Id", diskStoreDetails.getMemberId());
+ diskStoreData.accumulate("Disk Store Name", diskStoreDetails.getName());
+ diskStoreData.accumulate("Disk Store ID", diskStoreDetails.getId());
+ }
+
+ return ResultBuilder.buildResult(diskStoreData);
+ } else {
+ return ResultBuilder
+ .createInfoResult(CliStrings.LIST_DISK_STORE__DISK_STORES_NOT_FOUND_MESSAGE);
+ }
+ }
+
+ public Set<DistributedMember> getNormalMembers(final InternalCache cache) {
--- End diff --
Consider pulling this method into the Utils class, it's used here and in ShowMissginDiskStoresCommand. On the other hand you could inline the call to CliUtil.getAllNormalMembers; but since there seems to be some question as to it's purpose I would keep it as a utility method for now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131787085
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/UpgradeOfflineDiskStoreCommand.java ---
@@ -0,0 +1,179 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.apache.geode.management.internal.cli.commands.DiskStoreCommandsUtils.configureLogging;
--- End diff --
Remove import. See my previous moment regarding utility method calls.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #687: GEODE-3258: Refactoring DiskStoreCommands
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on the issue:
https://github.com/apache/geode/pull/687
With regards to your comments on the test classes and possible duplication:
I would consider splitting the existing test classes so that each of the new *Command classes has it's own tests. This may actually mean creating two test classes for each command - a Unit test to cover the functionality that doesn't need the overhead of the DUnit framework, and a DUnit test to cover what can't be accomplished in the Unit test.
I would also consider making this refactoring incrementally, with separate pull requests for each command that is extracted from the original DiskStoreCommands class. The incremtnal PR's would then entail creating a Command class and test classes as needed. As a starter, you may want to create the ...Utils class when you first extract a command that needs the methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---