Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Adds rollup runner and integration tests #341

Merged
merged 2 commits into from
Nov 18, 2020

Conversation

dbbaughe
Copy link
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

qreshi
qreshi previously approved these changes Nov 18, 2020
// TODO: Scenario: The rollup job is finished, but I (the user) want to redo it all again
/*
* TODO situations:
* There is a rollup.metadataID and doc but theres no job in target index?
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a heads up, I believe we do recover from this scenario now, however, it's not exactly as the TODO message expects:

  • During isJobValid, we'll validate source and target index
  • If we get the condition in this scenario, where rollup.metadataID and doc exist but there is no job in target index, we resolve it by updating the mappings for the target index and add the job in

So we recover from it but neither start over or move to FAILED, I suppose it would recover silently and continue from wherever the metadata has left off.

We can go over this behavior as part of reviewing retry logic later on to discuss if it's what we want or not.

logger.info("RollupMetadataException being thrown", e)
throw e
} catch (e: Exception) {
// TODO: Should update metadata and disable job here instead of allowing the rollup to keep going
Copy link
Contributor

Choose a reason for hiding this comment

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

This TODO should also be a candidate for inclusion whenever we work on retry logic.

@codecov
Copy link

codecov bot commented Nov 18, 2020

Codecov Report

Merging #341 (483abcd) into master (c1e0f1f) will increase coverage by 4.86%.
The diff coverage is 62.22%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #341      +/-   ##
============================================
+ Coverage     71.06%   75.92%   +4.86%     
- Complexity     1219     1296      +77     
============================================
  Files           181      181              
  Lines          6448     6538      +90     
  Branches        984     1004      +20     
============================================
+ Hits           4582     4964     +382     
+ Misses         1377     1040     -337     
- Partials        489      534      +45     
Impacted Files Coverage Δ Complexity Δ
...asticsearch/indexmanagement/rollup/RollupRunner.kt 68.96% <62.22%> (-23.35%) 24.00 <11.00> (+12.00) ⬇️
...nt/indexstatemanagement/model/destination/Chime.kt 40.90% <0.00%> (-13.64%) 2.00% <0.00%> (-2.00%)
...agement/indexstatemanagement/ManagedIndexRunner.kt 55.65% <0.00%> (+0.61%) 41.00% <0.00%> (ø%)
...ticsearch/indexmanagement/IndexManagementPlugin.kt 92.72% <0.00%> (+0.90%) 9.00% <0.00%> (ø%)
...asticsearch/indexmanagement/rollup/model/Rollup.kt 84.02% <0.00%> (+1.03%) 46.00% <0.00%> (+2.00%)
...ndexmanagement/rollup/model/dimension/Histogram.kt 62.26% <0.00%> (+1.88%) 14.00% <0.00%> (+1.00%)
...adata/TransportUpdateManagedIndexMetaDataAction.kt 88.88% <0.00%> (+3.70%) 10.00% <0.00%> (+1.00%)
...lup/action/explain/TransportExplainRollupAction.kt 66.66% <0.00%> (+6.24%) 6.00% <0.00%> (ø%)
...arch/indexmanagement/rollup/model/RollupMetrics.kt 85.07% <0.00%> (+8.95%) 30.00% <0.00%> (+6.00%)
...ticsearch/indexmanagement/IndexManagementRunner.kt 50.00% <0.00%> (+12.50%) 3.00% <0.00%> (+1.00%)
... and 11 more

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 c1e0f1f...2656cb7. Read the comment docs.

@dbbaughe dbbaughe merged commit 57c03fb into opendistro-for-elasticsearch:master Nov 18, 2020
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.

None yet

3 participants