GEODE-7698: Extracting a superclass of InternalDistributedMember #4590
Conversation
…dToString InternalDistributedMember has a dependency on this mutable static flag in it's to string, which is not ideal. It looks like the intention was to only shorten the hostname if it is dns name, not an ip address.
We were using flags and methods on SocketCreator to either change or cache the hostName that we read off the wire from the remote member. We don't want to do either of those things during deserialiation, we should just keep the hostname we read as is.
We need a concrete MemberIdentifier implementation in geode-membership to test membership. Extracting a superclass of InternalDistributedMember, MemberIdentifierImpl, which contains pretty much all of the InternalDistributedMember logic. InternalDistributedMember now just extends that class and implements DistributedMember.
Updating the expected POM, and fixing dependencies
This test is still using IDM, so it can't have a factory that creates MemberIdentifierImpl.
This class needs to directly implement the fromData_Pre_XXX methods.
Before the previous changes to InternalDistributedMember, we were flipping the hostname field to be an IP address when we deserialized the IDM, based on the resolve_dns flag. It looks like this was required to keep the HA region queue name generated from addFixedToString to match between old versions and the current version - the region name should have an IP address if network partition detection is enabled.
| @@ -244,19 +128,21 @@ public InternalDistributedMember(String i, int p) { | |||
| */ | |||
|
|
|||
| public InternalDistributedMember(ServerLocation location) { | |||
| super(MemberDataBuilder.newBuilder(getInetAddress(location), location.getHostName()) | |||
There was a problem hiding this comment.
Nice how having to call super forced us to put the extra logic into a method (getInetAddress).
...a/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java
Show resolved
Hide resolved
| @@ -494,4 +494,64 @@ public static void writePrimitiveClass(Class c, DataOutput out) throws IOExcepti | |||
| String.format("unexpected typeCode: %s", typeCode)); | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
ah I see why you moved this logic here from InternalDataSerializer: the latter is in geode-core and our new MemberIdentifierImpl is down in geode-membership and can't depend on stuff in core ✓
| return result; | ||
| } | ||
|
|
||
| public void addFixedToString(StringBuilder sb, boolean useIpAddress) { |
There was a problem hiding this comment.
now be a good time to give this method a meaningful name
| * or implied. See the License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| */ | ||
| package org.apache.geode.distributed.internal.membership.api; |
There was a problem hiding this comment.
gawd this (class) really goes into the membership API. That makes everything this class does part of the membership contract right?
class inheritance coming home to roost. I guess the alternative is to put a factory in the API and hide this impl in the membership module implementation package. Then InternalDistributedMember would have a MemberIdentifierImpl through a MemberIdentifierreference.
Problem with that idea is that MemberIdentifierImpl has a ton of methods not on MemberIdentifier. That implies we'd need another, wider, interface to implement those methods (maybe a sub-interface of MemberIdentifier) ugh.
sorry for the stream of consciousness feedback. I'll try to hone it and get back to you…
upthewaterspout
deleted the
feature/extract-member-identifier-superclass
branch
We need a concrete MemberIdentifier implementation in geode-membership to test membership.
Extracting a superclass of InternalDistributedMember, MemberIdentifierImpl, which contains pretty much all of the InternalDistributedMember logic. InternalDistributedMember now just extends that class and implements DistributedMember.