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

ServiceGraphBuilder #984

Merged
merged 18 commits into from May 16, 2019

Conversation

Projects
None yet
4 participants
@keeferrourke
Copy link
Collaborator

commented May 15, 2019

This PR adds two classes:

  • CoordinatedService2 which is intended to replace the existing CoordinatedService
  • ServiceGraphBuilder which is a builder class for Guava ServiceManagers that manage CoordinatedService2s

CoordinatedService2 adds the concept of "enhancements" in addition to the traditional downstream dependencies. Hopefully the documentation is clear as this was tricky to reason about at first!

@keeferrourke keeferrourke changed the title ServiceGraphBuilder WIP: ServiceGraphBuilder May 15, 2019

@mightyguava

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

cool stuff! What problem does it solve?

keeferrourke added some commits May 10, 2019

WIP: start order correct
infinite looping on the stop order when stopping enhancements
WIP: most tests passing
TODO:
 - merge cycle checks
 - ensure enhanced services shutdown completely before the dependent
   services
WIP: most tests passing again
TODO: Graph with only transitive enhancements is looping infinitely for
some reason :(
TODO: Failure to detect cycle when created by combo of enhancements and
dependencies.

@keeferrourke keeferrourke force-pushed the keefer.0509.coordinated_service_2 branch from c0ea2d4 to 2b67eac May 15, 2019

@keeferrourke keeferrourke changed the title WIP: ServiceGraphBuilder ServiceGraphBuilder May 15, 2019

updated docs and cleaned up implementation a bit
(pairing session with Jesse Wilson)
}
}

// Checks that each service registered with this builder has its dependencies registered.

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

Use /** */ KDoc comments

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

done in b6b460d

}
}

// Checks that no enhancement has been applied more than once.

This comment has been minimized.

Copy link
@swankjesse

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

done in b6b460d

// Checks that no enhancement has been applied more than once.
private fun validateEnhancementMap() {
val enhancementList = mutableListOf<Key<*>>()
for ((_, set) in enhancementMap.asMap()) {

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

for (set in enhancementMap.asMap().values())

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

or maybe just

enhancementList.addAll(enhancementMap.values())

?

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

done in b6b460d. went with += instead of addAll()


private var serviceMap = mutableMapOf<Key<*>, CoordinatedService2>() // map of all services
private val dependencyMap = LinkedHashMultimap.create<Key<*>, Key<*>>()
private val enhancementMap = LinkedHashMultimap.create<Key<*>, Key<*>>()

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

should this be a regular map? No duplicate keys right?

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

(and if you get multiple puts on the same key, you can fail on the 2nd put, not later in a validation step)

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

I could do this if I swap the mapping.

Right now it's service to set of enhancements, and the validation works by making sure no enhancement is in multiple sets.

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

Alright this has been updated in commit 5b34926. Note that ServiceGraphBuilder.addEnhancement() now throws from a check if there is an attempt to apply an enhancement to more than one service.

I also modified the CoordinatedService2 API to take varargs for addDependencies() and addEnhancements() respectively, since as a consequence of this change we are no longer always adding a list of services.

* @param service The identifier for the service that provides for `dependency`.
*/
fun addDependency(service: Key<*>, dependency: Key<*>) {
val dependencySet = dependencyMap[service]

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

dependencyMap.put(service, dependency)

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

done in b6b460d

return ServiceManager(serviceMap.values)
}

// Builds CoordinatedService2s from the instructions provided in the dependency and enhancement

This comment has been minimized.

Copy link
@swankjesse

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

done in b6b460d


for ((_, service) in serviceMap) {
val cycle = service.findCycle(validityMap)
if (cycle != null) {

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

replace if with check

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

done in b6b460d

private fun checkCycles() {
val validityMap = mutableMapOf<CoordinatedService2, CycleValidity>()

for ((_, service) in serviceMap) {

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

iterate on serviceMap.values()

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

done in b6b460d

// Checks that each service registered with this builder has its dependencies registered.
// (i.e. no one service requires a dependency or enhancement that doesn't exist.)
private fun validateDependencyMap() {
for ((big, dependents) in dependencyMap.asMap()) {

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

big –> service

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

done in b6b460d

// (i.e. no one service requires a dependency or enhancement that doesn't exist.)
private fun validateDependencyMap() {
for ((big, dependents) in dependencyMap.asMap()) {
if (serviceMap[big] == null) {

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

replace if with check?

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

done in b6b460d

keeferrourke added some commits May 15, 2019

changed SGBuilder.enhancementMap to mutableMap
The key-value relationship in ServiceGraphBuilder.enhancementMap has been
swapped.

Consequences of this:
 + this would've meant that each enhancement (value) would have been
   wrapped in listOf to fit the call to CS2.addEnhancements()
 + to keep the API symmetric for addEnhancements and addDependencies,
   these functions have been changed to take vararg parameters
   - I think this is nicer, but maybe it's not :shrug:
@keeferrourke

This comment has been minimized.

Copy link
Collaborator Author

commented May 15, 2019

@mightyguava: cool stuff! What problem does it solve?

Jesse and I added an example of a use case in the docstring for CoordinatedService2! Hopefully its a helpful illustration of the problem :)


/** Adds [services] as dependents downstream. */
fun addDependencies(vararg services: CoordinatedService2) {
directDependencies += services

This comment has been minimized.

Copy link
@mmihic

mmihic May 15, 2019

Collaborator

This seems dangerous. I don't think we want to allow dynamically added dependencies at runtime - dependencies should be built during graph initialization and the made immutable prior to service start. Ideally the service graph construction would occur outside the service itself such that by the time the service is created it is immutable; failing that we should be checking invariants here and failing if you attempt to add a dependency after the service is started.

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

@mmihic due to the self-relationships in the class, it doesn't seem possible to add enhancements/dependencies at construction time, but commit 10e38a8 adds runtime checks (and tests) that:

  • no CoordinatedService2 can wrap an already running Service
  • no CoordinatedService2 can have a dependency/enhancement added if it or the services to be added to it are not all NEW

Hopefully this addresses your concerns!

* This Service will stall in the `STARTING` state until all upstream services are `RUNNING`.
* Symmetrically it stalls in the `STOPPING` state until all dependent services are `TERMINATED`.
*/
internal class CoordinatedService2(val service: Service) : AbstractService() {

This comment has been minimized.

Copy link
@mmihic

mmihic May 15, 2019

Collaborator

Is there going to be fast-follow PR replacing of all uses of the existing CoordinatedService?

This comment has been minimized.

Copy link
@keeferrourke

keeferrourke May 15, 2019

Author Collaborator

I'm still new to the project and this PR was made with a lot of guidance from @swankjesse... But I believe that's the idea, yes.

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 15, 2019

Member

Yep. Kill CoordinatedService, then take its name.

prevented adding dependencies once graph is built
Added checks to `addDependencies()` and `addEnhancements()` methods to
ensure that neither the service nor the dependent services are running
when modifying the dependency graph.

Added a check to ensure that a CoordinatedService2 only wraps NEW
services.
* A service should be added before dependencies or enhancements are specified.
* Keys must be unique. If a key is reused, then the original key-service pair will be replaced.
*
* @param key A Guice Key used to identify the service.

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 16, 2019

Member

these docs aren’t useful, they are implied by the parameter types

@swankjesse swankjesse merged commit f382938 into master May 16, 2019

2 checks passed

ci/circleci: java Your tests passed on CircleCI!
Details
ci/circleci: node Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.