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

Remove Custom Listeners from SnapshotsService #37629

Merged

Conversation

original-brownbear
Copy link
Member

Motivations:
* Shorten the code
* Use ActionListener#wrap to get easy to reason about behavior in failure scenarios
* Remove duplication in the logic of handling snapshot completion listeners (listeners removing themselves and comparing snapshots to their targets)
* Also here, move all listener handling into SnapshotsService and remove custom listener class by putting listeners in a map

Motivations:
    * Shorten the code some more
    * Use ActionListener#wrap to get easy to reason about behavior in failure scenarios
    * Remove duplication in the logic of handling snapshot completion listeners (listeners removing themselves and comparing snapshots to their targets)
        * Also here, move all listener handling into `SnapshotsService` and remove custom listener class by putting listeners in a map
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@ywelsch
Copy link
Contributor

ywelsch commented Jan 22, 2019

@tbrooks8 can you review this? Thanks.

@Tim-Brooks
Copy link
Contributor

Yes. I will take a look at this tomorrow.

Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

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

Looks good. I do not understand the event change. But if you have a reason for that change just answer here so I understand and then feel free to merge.

@original-brownbear original-brownbear merged commit 36889e8 into elastic:master Jan 24, 2019
@original-brownbear original-brownbear deleted the nicer-snapshot-service branch January 24, 2019 09:11
@original-brownbear
Copy link
Member Author

@tbrooks8 thanks!

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 24, 2019
* elastic/master:
  Optimize warning header de-duplication (elastic#37725)
  Bubble exceptions up in ClusterApplierService (elastic#37729)
  SQL: Improve handling of invalid args for PERCENTILE/PERCENTILE_RANK (elastic#37803)
  Remove unused ThreadBarrier class (elastic#37666)
  Add built-in user and role for code plugin (elastic#37030)
  Consolidate testclusters tests into a single project (elastic#37362)
  Fix docs for MappingUpdatedAction
  SQL: Introduce SQL DATE data type (elastic#37693)
  disabling bwc test while backporting elastic#37639
  Mute ClusterDisruptionIT testAckedIndexing
  Set acking timeout to 0 on dynamic mapping update (elastic#31140)
  Remove index audit output type (elastic#37707)
  Mute FollowerFailOverIT testReadRequestsReturnsLatestMappingVersion
  [ML] Increase close job timeout and lower the max number (elastic#37770)
  Remove Custom Listeners from SnapshotsService (elastic#37629)
  Use m_m_nodes from Zen1 master for Zen2 bootstrap (elastic#37701)
  Fix index filtering in follow info api. (elastic#37752)
  Use project dependency instead of substitutions for distributions (elastic#37730)
  Update authenticate to allow unknown fields (elastic#37713)
  Deprecate HLRC EmptyResponse used by security (elastic#37540)
original-brownbear added a commit that referenced this pull request Jan 29, 2019
* Remove Custom Listeners from SnapshotsService

Motivations:
    * Shorten the code some more
    * Use ActionListener#wrap to get easy to reason about behavior in failure scenarios
    * Remove duplication in the logic of handling snapshot completion listeners (listeners removing themselves and comparing snapshots to their targets)
        * Also here, move all listener handling into `SnapshotsService` and remove custom listener class by putting listeners in a map
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants