GEODE_6883: Creating membership api classes #3985
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcmellawatt
force-pushed
the
feature/create-membership-api-GEODE-6883-2
branch
from
cd2462d
to
872bd11
Compare
echobravopapa
force-pushed
the
feature/create-membership-api-GEODE-6883-2
branch
from
a3eebcb
to
51c1e52
Compare
upthewaterspout
changed the title
upthewaterspout
requested review from
bschuchardt,
mcmellawatt and
echobravopapa
mcmellawatt
reviewed
Sep 9, 2019
...tegrationTest/java/org/apache/geode/distributed/internal/membership/MembershipJUnitTest.java
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/cache/server/internal/LoadMonitor.java
Outdated
Show resolved
Hide resolved
...che/geode/distributed/internal/membership/gms/locator/GMSLocatorRecoveryIntegrationTest.java
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
Show resolved
Hide resolved
...rc/main/java/org/apache/geode/distributed/internal/membership/gms/MembershipBuilderImpl.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/geode/distributed/internal/membership/gms/api/MembershipBuilder.java
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java
Outdated
Show resolved
Hide resolved
...java/org/apache/geode/distributed/internal/membership/gms/api/MembershipAPIArchUnitTest.java
Outdated
Show resolved
Hide resolved
Bill
requested changes
Sep 9, 2019
...re/src/main/java/org/apache/geode/distributed/internal/membership/adapter/ServiceConfig.java
Show resolved
Hide resolved
...re/src/main/java/org/apache/geode/distributed/internal/membership/adapter/ServiceConfig.java
Show resolved
Hide resolved
| MembershipManager create(); | ||
|
|
||
|
|
||
| static MembershipBuilder newMembershipBuilder(ClusterDistributionManager dm) { |
There was a problem hiding this comment.
It's generally poor design to have an interface depend on an implementation of the interface.
If MembershipBuilderImpl is indeed the only implementation we envision, then I recommend we sort of reverse the relationship in the following way:
- rename
MembershipBuilderto something likeMembershipBuilderInterface - rename
MembershipBuilderImpltoMembershipBuilder - move this method
newMembershipBuilder()down to what is now called theMembershipBuilderclass
There was a problem hiding this comment.
I agree having newMembershipBuilder() as part of the interface is somewhat odd. Why not just remove it all together and construct a MembershipBuilderImpl directly where needed? BTW, I think the current naming convention matches what we have for other interfaces/classes, so I'm not sure that needs to change (Cache/CacheImpl, Acceptor/AcceptorImpl, etc).
There was a problem hiding this comment.
Based on these suggestions, how about we just get rid of the MembershipBuilder interface entirely and rename the concrete MembershipBuilderImpl class to MembershipBuilder?
...rc/main/java/org/apache/geode/distributed/internal/membership/gms/MembershipBuilderImpl.java
Outdated
Show resolved
Hide resolved
...java/org/apache/geode/distributed/internal/membership/gms/api/MembershipAPIArchUnitTest.java
Outdated
Show resolved
Hide resolved
...java/org/apache/geode/distributed/internal/membership/gms/api/MembershipAPIArchUnitTest.java
Outdated
Show resolved
Hide resolved
Creating a new membership API package. Starting that package out with these classes: * MembershipBuilder - created by moving NetMember and NetLocator factory stuff out of MembershipFactory and renaming MembershipFactory to MembershipBuilder * Membership - Extracted an interface from MembershipManager * Authenticator - Moved Authenticator to the API package * MembershipStatistics - Extracted interface from DMStats * MembershipListener and MessageListener - Creating these new membership specific interfaces by splitting up DistributedMembershipListener into two separate interfaces. * MembershipConfig - Extracted interface from ServiceConfig * Adding an archunit test for the membership API. Ensure that the API only depends on other API classes. * Removing GMSMemberFactory and MemberServices These classes were adding another layer to creating a MembershipManager, but they didn't actually allow swapping in a different implementation. Inlining these methods into the respective NetLocatorFactory, NetMemberFactory, and MembershipManagerFactory. * Removing getDM from DirectChannelListener - We want to get rid of this interface in favor of other membership specific interfaces that don't depend on core.
upthewaterspout
force-pushed
the
feature/create-membership-api-GEODE-6883-2
branch
from
44ae648
to
27b213e
Compare
bschuchardt
reviewed
Sep 9, 2019
geode-core/src/distributedTest/java/org/apache/geode/distributed/LocatorDUnitTest.java
Show resolved
Hide resolved
geode-core/src/distributedTest/java/org/apache/geode/distributed/LocatorDUnitTest.java
Outdated
Show resolved
Hide resolved
bschuchardt
approved these changes
Sep 10, 2019
upthewaterspout
deleted the
feature/create-membership-api-GEODE-6883-2
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Creating a new membership API package. Starting that package out with
these classes:
MembershipBuilder - created by moving NetMember and NetLocator factory stuff
out of MembershipFactory and renaming MembershipFactory to MembershipBuilder
Membership - Extracted an interface from MembershipManager
Authenticator - Moved Authenticator to the API package
MembershipStatistics - Extracted interface from DMStats
MembershipListener and MessageListener - Creating these new membership
specific interfaces by splitting up DistributedMembershipListener into two
separate interfaces.
MembershipConfig - Extracted interface from ServiceConfig
Adding an archunit test for the membership API. Ensure that the API only
depends on other API classes.
Removing GMSMemberFactory and MemberServices
These classes were adding another layer to creating a MembershipManager,
but they didn't actually allow swapping in a different implementation.
Inlining these methods into the respective NetLocatorFactory,
NetMemberFactory, and MembershipManagerFactory.
Removing getDM from DirectChannelListener - We want to get rid of this
interface in favor of other membership
specific interfaces that don't depend on core.