Skip to content

[GEODE-5971] Refactor offline disk store commands to extend SingleGfshCommand base type#2794

Merged
jdeppe-pivotal merged 2 commits intoapache:developfrom
aditya87:geode-5971-offline-diskstore-commands
Nov 7, 2018
Merged

[GEODE-5971] Refactor offline disk store commands to extend SingleGfshCommand base type#2794
jdeppe-pivotal merged 2 commits intoapache:developfrom
aditya87:geode-5971-offline-diskstore-commands

Conversation

@aditya87
Copy link
Contributor

@aditya87 aditya87 commented Nov 6, 2018

Signed-off-by: Peter Tran ptran@pivotal.io

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?

Note:

Please ensure that once the PR is submitted, you check travis-ci 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.

…hCommand base type

Signed-off-by: Peter Tran <ptran@pivotal.io>

@Category({PersistenceTest.class})
public class DescribeDiskStoreCommandIntegrationTest {
public class DescribeDiskStoreCommandDUnitTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is really an integration test. Tests that use ServerStarterRule (or LocatorStarterRule) don't fork separate JVMs but start a cache within the test JVM itself - this is essentially the definition of an integration test as it applies to the codebase. ClusterStarterRule, on the other hand, does fork separate JVMs and thus all tests that use that rule should be classified as DUnit tests.

import org.apache.geode.management.internal.cli.result.model.ResultModel;

public class AlterOfflineDiskStoreCommand extends InternalGfshCommand {
public class AlterOfflineDiskStoreCommand extends SingleGfshCommand {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this would make sense. these are offline commands. SingleGfshCommand is designed for commands that would change cluster configuration. I would like to see all offline commands stay as GfshCommand or InternalGfshCommand..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some offline conversation, the ultimate goal would be to move everything to SingleGfshCommand.

@jdeppe-pivotal jdeppe-pivotal merged commit ae8abe2 into apache:develop Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants