Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GEODE-6558: Add InternalDistributedSystem.Builder #3354

Merged

Conversation

kirklund
Copy link
Contributor

@kirklund kirklund commented Mar 25, 2019

  • Remove all but one constructor.
  • Replace newInstance methods with InternalDistributedSystem.Builder.
  • Replace newInstanceForTesting methods with
    InternalDistributedSystem.BuilderForTesting.
  • Replace public static boolean var ALLOW_MULTIPLE_SYSTEMS with usage
    of Boolean.getBoolean(ALLOW_MULTIPLE_SYSTEMS_PROPERTY) in
    connectInternal.

Most code paths are using DistributedSystem.connect or
InternalDistributedSystem.connectInternal to construct a system.
connectInternal now uses InternalDistributedSystem.Builder.

Several tests are using InternalDistributedSystem.BuilderForTesting.

NOTES FOR REVIEWERS (PLEASE READ):

NOTE: this is NOT the final refactoring/change to InternalDistributedSystem involving the Builder. We need to make some changes to reconnect for Micrometer. We will also be injecting a Micrometer MeterRegistry into InternalDistributedSystem. And finally, I think we'll be looking into making connectInternal private and changing internal code paths to use the Builder instead of connectInternal. Then there would be two code paths to create a InternalDistributedSystem:

  1. DistributedSystem.connect which would delegate to InternalDistributedSystem.Builder
  2. InternalDistributedSystem.Builder which would be used from internal code paths such as InternalCacheBuilder and reconnect

ALSO: I'd love to delete BuilderForTesting and replace it with mocking in every test where that's possible.

Please review: @demery-pivotal @mhansonp

@kirklund
Copy link
Contributor Author

We could probably merge the two Builders if we add setInitialize(boolean) which defaults to true. The tests that use BuilderForTesting are basically constructing an instance of InternalDistributedSystem which NEVER gets initialized.

@kirklund
Copy link
Contributor Author

The StressNewTest jobs failed multiple runs of MultipleCacheJUnitTest.

@kirklund kirklund force-pushed the GEODE-6558-InternalDistributedSystem-builder branch from c984214 to 1412f01 Compare March 26, 2019 23:48
@kirklund
Copy link
Contributor Author

I had to fix both GEODE-6562 and GEODE-6565 to get this precheckin green. Those are the 2nd and 3rd commits in this PR.

@kirklund
Copy link
Contributor Author

SecurityManagerLifecycleIntegrationTest failed due to changes in the PR.

@kirklund
Copy link
Contributor Author

LuceneClientSecurityWithRegionCreatedBeforeIndexDUnitTest and LuceneClientSecurityDUnitTest both failed, possibly due to changes in this PR.

@kirklund
Copy link
Contributor Author

kirklund commented Mar 27, 2019

I separated GEODE-6562 and GEODE-6565 out to their own PRs.

InternalDistributedSystemBuilderIntegrationTest uses security which revealed a Shiro ThreadLocal leak when a Cache is not created (GEODE-6565) -- this caused MultipleCacheJUnitTest to fail when run later in the same JVM.

Fixing GEODE-6565 (Shiro ThreadLocal leak) caused LuceneClientSecurityDUnitTest and subclass to fail because a test was trying to use gemfire.properties that are incompatible with what's specified in getDistributedSystemProperties which is overridden in the super class.

* Remove all but one constructor.
* Replace newInstance methods with InternalDistributedSystem.Builder.
* Replace newInstanceForTesting methods with
InternalDistributedSystem.BuilderForTesting.
* Replace public static boolean var ALLOW_MULTIPLE_SYSTEMS with usage
of Boolean.getBoolean(ALLOW_MULTIPLE_SYSTEMS_PROPERTY) in
connectInternal.

Most code paths are using DistributedSystem.connect or
InternalDistributedSystem.connectInternal to construct a system.
connectInternal now uses InternalDistributedSystem.Builder.

Several tests are using  InternalDistributedSystem.BuilderForTesting.
@kirklund kirklund force-pushed the GEODE-6558-InternalDistributedSystem-builder branch from 1412f01 to d80439a Compare March 28, 2019 16:20
@kirklund
Copy link
Contributor Author

DistributedTest failure is an unrelated flaky failure in ClientServerTransactionFailoverDistributedTest

Copy link
Member

@moleske moleske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll merge those two builders in a future commit

@kirklund kirklund merged commit d7911d6 into apache:develop Mar 29, 2019
@kirklund kirklund deleted the GEODE-6558-InternalDistributedSystem-builder branch March 29, 2019 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants