Skip to content
This repository has been archived by the owner on Sep 28, 2021. It is now read-only.

Manage lifecycle of installed modules #357

Merged
merged 3 commits into from
Jun 2, 2021
Merged

Manage lifecycle of installed modules #357

merged 3 commits into from
Jun 2, 2021

Conversation

i-maravic
Copy link
Contributor

When an ApolloModule installs another ApolloModule, it should start managing lifecycles for all the keys from the installed ApolloModule.

Apollo will not manage the installed modules on the service shutdown if we don't do this.

@i-maravic
Copy link
Contributor Author

@lepistone

@i-maravic i-maravic changed the base branch from master to 1.x May 24, 2021 11:50
@i-maravic i-maravic changed the base branch from 1.x to master May 24, 2021 11:51
@i-maravic i-maravic changed the base branch from master to 1.x May 24, 2021 12:03
When an ApolloModule installs another ApolloModule, it should start managing lifecycles for all the keys from the installed ApolloModule.

Apollo will not manage the installed modules on the service shutdown if we don't do this.
@codecov
Copy link

codecov bot commented May 24, 2021

Codecov Report

Merging #357 (9b2fcd7) into 1.x (10b98e0) will increase coverage by 0.09%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##                1.x     #357      +/-   ##
============================================
+ Coverage     72.04%   72.14%   +0.09%     
- Complexity      673      676       +3     
============================================
  Files           137      137              
  Lines          2862     2868       +6     
  Branches        167      169       +2     
============================================
+ Hits           2062     2069       +7     
+ Misses          763      761       -2     
- Partials         37       38       +1     
Impacted Files Coverage Δ
...om/spotify/apollo/module/AbstractApolloModule.java 88.88% <83.33%> (+13.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10b98e0...9b2fcd7. Read the comment docs.

instance.waitForShutdown();
}

assertThat(childCreated.get(), is(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

nitty-pick: What do you think about repeating this at line 462? Something like this could be possible:

try (Service.Instance instance = service.start()) {
    assertThat(childCreated.get(), is(true));
    assertThat(childClosed.get(), is(false));
    instance.getSignaller().signalShutdown();  
    instance.waitForShutdown();    
}

From what I am looking in the code I think it should be fine as they are set in the moment you're creating the instance of the class. What do you think?

@klaraward
Copy link
Contributor

klaraward commented Jun 2, 2021

Reviewed and approved by weaver squad.
<jinx>Since the keys are stored in a ImmutableSet and then a LinkedHashSet we are positive the ordering of the lifecycle managed keys is preserved and this PR should not lead to any big surprises in the shutdown sequence of an Apollo service.</jinx>

@klaraward klaraward merged commit b6dc23d into spotify:1.x Jun 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants