GEODE-7634: create an ArchUnitTest that asserts that geode-core only … #4549

bschuchardt · 2019-12-30T21:24:38Z

…uses the membership API

creating a new test to enforce use of membership APIs.

This PR also deletes old deprecated loadEmergencyClasses methods and associated
state from a number of classes. That functionality was never integrated fully into membership
anyway.

Membership API classes and interfaces are now in membership.api instead of being buried in membership.gms.api. This simplifies ArchUnitTest rules.

HostAddress is duplicated and the copy is named LocatorAddress. This new class is
specifically for client cache connection pool use. The HostAddress class is retained and is now used
only by membership.

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, 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.

echobravopapa

Package rename, is good. NASA won't like the lack of emergency classes, but we'll worry about that next year...
duplication of LocatorAddress is my only nitpik

echobravopapa · 2019-12-31T22:38:10Z

...tegrationTest/java/org/apache/geode/distributed/internal/membership/MembershipJUnitTest.java

@@ -363,15 +358,15 @@ public void testLocatorAndTwoServersJoinUsingDiffeHellman() throws Exception {
      // manager queues new views for processing through the DM listener,
      // which is a mock object in this test
      System.out.println("waiting for views to stabilize");
-      JoinLeave jl1 = ((GMSMembership) m1).getServices().getJoinLeave();
-      JoinLeave jl2 = ((GMSMembership) m2).getServices().getJoinLeave();
+      final Membership mgr1 = m1;


like this, test at the API boundary!

echobravopapa · 2019-12-31T22:41:17Z

.../apache/geode/distributed/internal/membership/api/CoreOnlyUsesMembershipAPIArchUnitTest.java

+ * This test class can be removed if and when we create an isolated Java module that does
+ * not export internal membership classes.
+ */
+public class CoreOnlyUsesMembershipAPIArchUnitTest {


I like how this is segmented, very much. only concern is that a new package ( or renamed package) will require manual entry into this list- aka how do we go about having an automated way to catch any new violations?

I couldn't figure that one out. Any attempts met with OOME when run from gradle. That concern will go away when we start using Jigsaw and enforce API use.

I figured as much, just something to keep an eye on...

echobravopapa · 2019-12-31T23:05:42Z

geode-core/src/main/java/org/apache/geode/cache/client/internal/LocatorAddress.java

+
+import org.apache.commons.validator.routines.InetAddressValidator;
+
+public class LocatorAddress {


this looks like a clone of HostAddress, curious why we want to have both classes...?

seems like avoidable duplication

I'll do something about that. I don't much like it either.

I'm going to move LocatorAddress to the geode-tcp-server module and subclass it in membership for HostAddress.

echobravopapa · 2019-12-31T23:14:06Z

geode-core/src/main/java/org/apache/geode/distributed/internal/Distribution.java

@@ -114,32 +112,6 @@ void startupMessageFailed(InternalDistributedMember mbr,

  Set<InternalDistributedMember> getMembersNotShuttingDown();

-  // TODO - this method is only used by tests


Nice to see test hooks reducing.

echobravopapa · 2019-12-31T23:14:31Z

geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionImpl.java

@@ -638,49 +612,6 @@ public DistributedMember getCoordinator() {
    return membership.getMembersNotShuttingDown();
  }

-  // TODO - this method is only used by tests


…uses the membership API creating a new test to enforce use of membership APIs. There are a few exceptions in test code, plus others that aren't in the "integration test" source set.

echobravopapa

apologies if I caused those conflicts with my latest merge

…membership

apache#4549) * GEODE-7634: create an ArchUnitTest that asserts that geode-core only uses the membership API creating a new test to enforce use of membership APIs. There are a few exceptions in test code, plus others that aren't in the "integration test" source set. * removing test dependencies and fixing crashDistributedSystem method * moved membership API classes to membership.api package and fixed OOME * move address verification class to tcp-server module and subclass in membership * rebasing on Ernie's GeodeGlossary stuff

bschuchardt requested review from echobravopapa and upthewaterspout December 31, 2019 21:02

bschuchardt marked this pull request as ready for review December 31, 2019 21:14

echobravopapa requested changes Dec 31, 2019

View reviewed changes

bschuchardt requested a review from echobravopapa January 1, 2020 00:01

bschuchardt added 2 commits December 31, 2019 16:07

GEODE-7634: create an ArchUnitTest that asserts that geode-core only …

cbb20bd

…uses the membership API creating a new test to enforce use of membership APIs. There are a few exceptions in test code, plus others that aren't in the "integration test" source set.

removing test dependencies and fixing crashDistributedSystem method

d9b97cf

echobravopapa approved these changes Jan 1, 2020

View reviewed changes

bschuchardt added 3 commits December 31, 2019 16:09

moved membership API classes to membership.api package and fixed OOME

b141b59

move address verification class to tcp-server module and subclass in …

b3b5f9b

…membership

rebasing on Ernie's GeodeGlossary stuff

f2d7ab8

bschuchardt force-pushed the feature/GEODE-7634 branch from f939b33 to f2d7ab8 Compare January 1, 2020 00:17

bschuchardt merged commit 263b3e2 into develop Jan 1, 2020

bschuchardt deleted the feature/GEODE-7634 branch January 1, 2020 03:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-7634: create an ArchUnitTest that asserts that geode-core only … #4549

GEODE-7634: create an ArchUnitTest that asserts that geode-core only … #4549

bschuchardt commented Dec 30, 2019 •

edited

Loading

echobravopapa left a comment

echobravopapa Dec 31, 2019

echobravopapa Dec 31, 2019

bschuchardt Dec 31, 2019

echobravopapa Dec 31, 2019

echobravopapa Dec 31, 2019

echobravopapa Dec 31, 2019

bschuchardt Dec 31, 2019

bschuchardt Dec 31, 2019

echobravopapa Dec 31, 2019

echobravopapa Dec 31, 2019

echobravopapa left a comment


		import org.apache.commons.validator.routines.InetAddressValidator;

		public class LocatorAddress {

		@@ -114,32 +112,6 @@ void startupMessageFailed(InternalDistributedMember mbr,

		Set<InternalDistributedMember> getMembersNotShuttingDown();

		// TODO - this method is only used by tests

GEODE-7634: create an ArchUnitTest that asserts that geode-core only … #4549

GEODE-7634: create an ArchUnitTest that asserts that geode-core only … #4549

Conversation

bschuchardt commented Dec 30, 2019 • edited Loading

For all changes:

Note:

echobravopapa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echobravopapa left a comment

Choose a reason for hiding this comment

bschuchardt commented Dec 30, 2019 •

edited

Loading