You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "echobravopapa (GitHub)" <gi...@apache.org> on 2019/11/12 00:22:34 UTC
[GitHub] [geode] echobravopapa opened pull request #4313: Moving
GMSMembershipManager into gms package
Adding a MembershipManagerAdapter class that ClusterDistributionManager and the
rest of geode-core uses, which wraps GMSMembershipManager. Moving
GMSMembershipManager into the gms package.
MembershipManagerAdapter now owns the DirectChannel, rather than GMSMembershipManager.
Removing the MembershipManager interface and replacing uses of it with the
Membership interface.
Currently, lots of dependency violiations in MembershipDependenciesJUnitTest!
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.
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
class name?
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] upthewaterspout commented on pull request #4313:
GEODE-7456: Moving GMSMembershipManager into gms package
Posted by "upthewaterspout (GitHub)" <gi...@apache.org>.
Sorry, I mispoke. We did add an ArchUnit test - see DistributionArchUnitTest.
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
ditto
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
It appears to me that this could be made final if the it was set in the constructor instead of `startDirectChannel()`.
The need to be able to override it for testing (mentioned around `setDirectChannel()` could be addressed via the constructor.
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
ditto previous comment
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
There is no `MembershipManagerAdapter` now right?
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
TODO when? do we need a ticket instead?
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] upthewaterspout closed pull request #4313:
GEODE-7456: Moving GMSMembershipManager into gms package
Posted by "upthewaterspout (GitHub)" <gi...@apache.org>.
[ pull request closed by upthewaterspout ]
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] echobravopapa commented on pull request #4313:
GEODE-7456: Moving GMSMembershipManager into gms package
Posted by "echobravopapa (GitHub)" <gi...@apache.org>.
this is accurate
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
I understand the intention here is to ensure that nothing in `geode-core` uses `org.apache.geode.distributed.internal.membership.gms.api.Membership` directly—that within `geode-core` all membership functionality is delivered by `MembershipManagerAdapter` (sic). If that's right, it might be a good idea to add that ArchUnit test to this PR.
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
This is a big step backward. I feel like we should have tickets covering these new dependencies before we merge this PR.
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
shouldn't this be called `getMembership()` now?
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #4313: GEODE-7456:
Moving GMSMembershipManager into gms package
Posted by "Bill (GitHub)" <gi...@apache.org>.
class name
[ Full content available at: https://github.com/apache/geode/pull/4313 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org