Skip to content

GEODE-6027: refactor updateClusterConfig#2825

Closed
pdxrunner wants to merge 1 commit intoapache:developfrom
pdxrunner:GEODE-6027
Closed

GEODE-6027: refactor updateClusterConfig#2825
pdxrunner wants to merge 1 commit intoapache:developfrom
pdxrunner:GEODE-6027

Conversation

@pdxrunner
Copy link
Copy Markdown

  • Add ResultModel parameter
  • change from void return to boolean

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.

- Add ResultModel parameter
- change from void return to boolean
@pdxrunner pdxrunner requested a review from jinmeiliao November 9, 2018 19:08
@jdeppe-pivotal
Copy link
Copy Markdown
Contributor

@aditya87 @Petahhh FYI on this PR
Although it might seem pointless to separate the ResultModel and configObject as params, my feeling is that these don't belong together and the only reason RM contains the configObject is because the commands return a RM. To that end, I'm also considering having commands return a Pair<ResultModel, Object> (http://commons.apache.org/proper/commons-lang/javadocs/api-3.8/index.html?org/apache/commons/lang3/tuple/Pair.html) allowing us to separate the two concerns. Thoughts?

@Override
public void updateClusterConfig(String group, CacheConfig cacheConfig, Object element) {
public boolean updateClusterConfig(String group, CacheConfig cacheConfig, Object element,
ResultModel resultModel) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this resultModel used in any implementation?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not yet. We wanted to due the refactoring of updateClusterConfig as an initial PR. Individual commands will use this as they are refactored to use SingleGfshCommand. Initial work on refactoring AlterAsyncEventQueue is what prompted this change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmm, yes it may well be a premature refactor that we don't really need.

.stream()
.filter(regionConfig -> regionConfig.getName().equals(newCacheElement.getRegionName()))
.forEach(regionConfig -> regionConfig.getCustomRegionElements().add(newCacheElement));
return true;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

where is the return value used?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The return value is used in CommandExecutor.invokeCommand. Individual command classes that override updateClusterConfig will return true or false to indicate whether the configuration was updated or not (or possibly throw an exception if an error occurred updating the configuration).

@jake-at-work
Copy link
Copy Markdown
Contributor

@pdxrunner is this PR going to see any more effort or should it be closed?

@pdxrunner
Copy link
Copy Markdown
Author

@pdxrunner is this PR going to see any more effort or should it be closed?

Thanks for pinging me on this. I should have closed it out when I superseded it with #2846

@pdxrunner pdxrunner closed this Jan 3, 2019
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.

4 participants