You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "aditya87 (GitHub)" <gi...@apache.org> on 2018/11/14 23:30:56 UTC
[GitHub] [geode] aditya87 opened pull request #2854: GEODE-5871:
Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Co-authored-by: Jens Deppe <jd...@pivotal.io>
Co-authored-by: Kenneth Howe <kh...@pivotal.io>
Co-authored-by: Aditya Anchuri <aa...@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:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [X] Is your initial contribution a single, squashed commit?
- [X] Does `gradlew build` run cleanly?
- [X] 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, 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.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on pull request #2854:
GEODE-5971: Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "pdxrunner (GitHub)" <gi...@apache.org>.
Good suggestion!
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on issue #2854: GEODE-5971:
Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "pdxrunner (GitHub)" <gi...@apache.org>.
@jinmeiliao @aditya87 Good point. Our current implementation will always use the _all groups_ list for updating the configuration, not just those specified with a `--group` option.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2854:
GEODE-5971: Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
Looks good with the new marker interface.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on issue #2854: GEODE-5971:
Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "pdxrunner (GitHub)" <gi...@apache.org>.
Removed the 2nd (confusing) `updateConfig` method from `SingleGfshCommand` in favor of using a marker interface as has been suggested.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #2854:
GEODE-5871: Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
I like the overall general approach, however I don't like that the code flow here does not make it clear that `updateCacheConfig` and `updateConfigForGroup` are mutually exclusive.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2854:
GEODE-5971: Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
I suggest we name this as `UpdateAllConfigurationGroupsCommandMarker` or something, to make it clear that this interface is a marker and thus deliberately empty, otherwise we'd be lying about its behavior w.r.t to the name.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2854: GEODE-5971:
Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
@aditya87 that is an interesting case, I think in general, when a group is provided in the command, then we should just pass that config to the interface method. The marker is used to differentiate only when there is no group supplied in the command, that determines should it be default to "cluster" group or apply to all groups.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2854:
GEODE-5971: Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
For better readability:
if(!StringUtils.isBlank){
// use the group
}
else if (command is instance of UpdateAllMaker){
// pass all groups
}
else{
// pass in "cluster"
}
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on issue #2854: GEODE-5971:
Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
Like the marker interface change 👍
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2854: GEODE-5871:
Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
How do you differentiate which interface method to call? in what situation we should update just one config object and in what situation we should go through all of them? I think this is determined by the nature of the command itself. Like "create aeq" needs to operate on only one group, while "delete aeq" needs to go through all groups and find the right one to delete.
So the command needs to somehow indicate what kind of operation this is. We had suggested using a marker interface. If a command doesn't have that marker interface, then it's just doing what it is now, otherwise, the command executor will get all the configs and call the updateConfig method with each group's config. The interface of SingleGfshCommand doesn't need to change.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal closed pull request #2854:
GEODE-5971: Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
[ pull request closed by jdeppe-pivotal ]
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2854:
GEODE-5971: Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
I suggest we name this as `UpdateAllConfigurationGroupsMarker`, to make it clear that this interface is a marker and thus deliberately empty, otherwise we'd be lying about its behavior w.r.t to the name.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on issue #2854: GEODE-5971:
Refactor AlterAsycnEventQueueCommand to use SingleGfshCommand
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
@jinmeiliao Sure, I agree in general terms that we should set some kind of marker interface -- there's an interesting case where the destroy aeq command can run on a single config or can search through all configs, depending on whether the user provided a group option. We'll have to flush out how that could work.
[ Full content available at: https://github.com/apache/geode/pull/2854 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org