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

Fix GetSnapshots to not return non-existent snapshots with ignore_unavailable=true #6839

Merged
merged 4 commits into from
Apr 6, 2023

Conversation

gaobinlong
Copy link
Collaborator

@gaobinlong gaobinlong commented Mar 27, 2023

Description

Relates to #6820.

Currently, the Get Snapshot API returns all of the snapshots in-progress when the specified snapshot is non-existent and the parameter ignore_unavailable is set to true.

The main changes are:

  1. Fix the bug for Get Snapshot API, return empty result when getting a non-existing snapshot with setting ignore_unavailable to true and there are just some snapshots in-progress.
  2. Add some test code for the change.

Issues Resolved

#6820.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…a non-existing snapshot (opensearch-project#6820)

Signed-off-by: Gao Binlong <gbinlong@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2023

Codecov Report

Merging #6839 (f128d73) into main (8c9cfdc) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6839      +/-   ##
============================================
- Coverage     70.73%   70.71%   -0.02%     
+ Complexity    59281    59235      -46     
============================================
  Files          4812     4812              
  Lines        283614   283614              
  Branches      40896    40897       +1     
============================================
- Hits         200606   200550      -56     
- Misses        66552    66628      +76     
+ Partials      16456    16436      -20     
Impacted Files Coverage Δ
...ter/snapshots/get/TransportGetSnapshotsAction.java 41.58% <0.00%> (ø)

... and 482 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Gao Binlong <gbinlong@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dblock
Copy link
Member

dblock commented Mar 27, 2023

Shouldn't specifying a non-existent snapshot be an error? (different from an unavailable snapshot)

@gaobinlong
Copy link
Collaborator Author

@dblock just like Get index API or Get alias API, I think the common parameter ignore_unavailable contains the meaning of ignoring missing, by going through the code of Get Snapshot API I found that's the same logic, when the specified snapshot is non-existent, the api will throw SnapshotMissingException if ignore_unavailable is false(by default), but return empty result if ignore_unavailable is true (has a bug when there are some snapshots in-progress, this is what this PR wants to fix).

@anasalkouz anasalkouz linked an issue Mar 28, 2023 that may be closed by this pull request
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification. Is there a unit test possible for this?

CHANGELOG.md Outdated Show resolved Hide resolved
@gaobinlong gaobinlong changed the title Fix bug for Get Snapshot API to return correct response when getting a non-existing snapshot Fix GetSnapshots to not return non-existent snapshots with ignore_unavailable=true Apr 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testRelocateWithQueuedOperationsDuringHandoff
      1 org.opensearch.indices.replication.SegmentReplicationIT.testScrollCreatedOnReplica

@gaobinlong
Copy link
Collaborator Author

Thanks for the clarification. Is there a unit test possible for this?

Thanks for your review and suggestion, I've tried adding unit tests for the changed method but found repositoryService is referenced in the method and is hard to mock:

final Repository repository = repositoriesService.repository(repositoryName);
, so maybe we can only add integration test for this change and it seems that most snapshots related operations only have integration test.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Let's merge. Consider some refactoring to make unit tests possible.

@dblock dblock merged commit 8b34e5f into opensearch-project:main Apr 6, 2023
@dblock dblock added the backport 2.x Backport to 2.x branch label Apr 6, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 6, 2023
…vailable=true (#6839)

* Fix bug for Get Snapshot API to return correct response when getting a non-existing snapshot (#6820)

Signed-off-by: Gao Binlong <gbinlong@amazon.com>

* modify change log

Signed-off-by: Gao Binlong <gbinlong@amazon.com>

* Modify changelog

Signed-off-by: Gao Binlong <gbinlong@amazon.com>

---------

Signed-off-by: Gao Binlong <gbinlong@amazon.com>
(cherry picked from commit 8b34e5f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dblock pushed a commit that referenced this pull request Apr 6, 2023
…vailable=true (#6839) (#7029)

* Fix bug for Get Snapshot API to return correct response when getting a non-existing snapshot (#6820)



* modify change log



* Modify changelog



---------


(cherry picked from commit 8b34e5f)

Signed-off-by: Gao Binlong <gbinlong@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Get snapshot API returns incorrect response
3 participants