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

Fixes snapshot bugs #244

Merged
merged 8 commits into from
Jun 25, 2020
Merged

Conversation

dbbaughe
Copy link
Contributor

Issue #, if available:
#240

Description of changes:
There were a few bugs found in snapshot from the #240 issue.

  1. The snapshot name was not persisting across multiple WaitForSnapshotStep executions as it was added to the info object and not the ActionProperties.
  2. The ConcurrentSnapshotExecutionException that was being caught in AttemptSnapshotStep was not working correctly in multi-node setups as the exception is wrapped in a RemoteTransportException
  3. There was no try/catch around the WaitForSnapshotStep execute method

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

@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #244 into master will increase coverage by 0.31%.
The diff coverage is 43.75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #244      +/-   ##
============================================
+ Coverage     70.80%   71.12%   +0.31%     
- Complexity      605      615      +10     
============================================
  Files            83       83              
  Lines          3552     3584      +32     
  Branches        587      593       +6     
============================================
+ Hits           2515     2549      +34     
+ Misses          740      729      -11     
- Partials        297      306       +9     
Impacted Files Coverage Δ Complexity Δ
...atemanagement/step/snapshot/AttemptSnapshotStep.kt 48.27% <18.18%> (-4.79%) 3.00 <1.00> (ø)
...atemanagement/step/snapshot/WaitForSnapshotStep.kt 55.31% <48.48%> (+41.98%) 4.00 <1.00> (+3.00)
...ent/model/managedindexmetadata/ActionProperties.kt 85.71% <88.88%> (+5.71%) 7.00 <2.00> (+4.00)
...ch/indexstatemanagement/model/destination/Chime.kt 14.28% <0.00%> (-42.86%) 3.00% <0.00%> (-1.00%)
...ement/step/notification/AttemptNotificationStep.kt 62.06% <0.00%> (-10.35%) 5.00% <0.00%> (ø%)
...ticsearch/indexstatemanagement/model/Transition.kt 80.39% <0.00%> (ø) 5.00% <0.00%> (ø%)
...ndexstatemanagement/IndexStateManagementHistory.kt 80.19% <0.00%> (+1.98%) 28.00% <0.00%> (+1.00%)
...exstatemanagement/model/destination/Destination.kt 47.05% <0.00%> (+4.41%) 6.00% <0.00%> (ø%)
...arch/indexstatemanagement/action/SnapshotAction.kt 46.15% <0.00%> (+15.38%) 4.00% <0.00%> (+2.00%)
... and 1 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 b252f99...587f8e5. Read the comment docs.

@dbbaughe
Copy link
Contributor Author

codecov/path failing because of coverage of the extra branches added in the logic.
Since these are fixing critical bugs in snapshot, will get these in first to make it for the 1.9 release and add additional unit tests after.

@dbbaughe dbbaughe requested review from qreshi and bowenlan-amzn June 25, 2020 16:11
@dbbaughe dbbaughe merged commit b4ab545 into opendistro-for-elasticsearch:master Jun 25, 2020
dbbaughe added a commit to dbbaughe/index-management that referenced this pull request Jul 29, 2020
* Simplifies snapshot message

* Handle ConcurrentSnapshotExecutionExceptions during remote transport calls

* Adds try/catch block around WaitForSnapshotStep execute

* Fixes snapshot completed check including failed/aborted and makes status check exhaustive

* Adds snapshot name to ActionProperties

* Uses snapshotName in ActionProperties in snapshot steps

* Adds a couple more integration tests for snapshot action

* Suppress complex method in execute step
dbbaughe added a commit that referenced this pull request Jul 29, 2020
* Fixes snapshot bugs (#244)

* Simplifies snapshot message

* Handle ConcurrentSnapshotExecutionExceptions during remote transport calls

* Adds try/catch block around WaitForSnapshotStep execute

* Fixes snapshot completed check including failed/aborted and makes status check exhaustive

* Adds snapshot name to ActionProperties

* Uses snapshotName in ActionProperties in snapshot steps

* Adds a couple more integration tests for snapshot action

* Suppress complex method in execute step

* Fixes snapshot issues, adds history mapping update workflow, adds tests (#255)

* Fixes snapshot issues, adds history mapping update workflow, adds tests

* Adds a couple more tests for the destination parsing tests

* Removes unneeded line
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