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.

Sign up for GitHub

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

Jump to bottom

[Remote Store] Add snapshot type information in repository data. #13827

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Remote Store] Add snapshot type information in repository data. #13827

wants to merge 1 commit into from

Conversation

harishbhakuni
Copy link
Contributor

@harishbhakuni harishbhakuni commented May 25, 2024 •
edited
Loading

Description

  • While adding Shallow copy snapshots support, we introduced a new method deleteSnapshotsAndReleaseLockFiles (which also takes care of releasing remote store lock for the snapshot) in repository interface. We did not directly used this new method in snapshot service as it would be a breaking change for existing users who have implemented this interface differently or for those who do not have this method implemented in their repository implementation.
  • Thus, during snapshot deletion we relied on shallow_copy_snapshot_enabled repository setting flag to decide if we need to use the old method deleteSnapshots or the new method deleteSnapshotsAndReleaseLockFiles.
  • As we will be supporting migration from remote store indices to docrep indices soon, we cannot rely on this repository setting as it would end up into dangling lock files in remote store.
  • As part of this PR, adding snapshot type info in repository data and using that in to determine if new method deleteSnapshotsAndReleaseLockFiles should be invoked or not from snapshot service.

Related Issues

Resolves #8610

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • API changes companion pull request created.
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

@github-actions github-actions bot added bug Something isn't working Storage:Durability Issues and PRs related to the durability framework labels May 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 4d8fa9c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@prudhvigodithi prudhvigodithi mentioned this pull request Jun 18, 2024
Closed
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Jun 25, 2024
@opensearch-ci-bot opensearch-ci-bot mentioned this pull request Jun 27, 2024
Open
gbbafna
gbbafna reviewed Jul 29, 2024
View reviewed changes
server/src/main/java/org/opensearch/snapshots/SnapshotType.java Show resolved Hide resolved
server/src/main/java/org/opensearch/snapshots/SnapshotsService.java
Comment on lines +2243 to +2244
boolean cleanupRemoteStoreLockFiles = snapshotIds.stream()
.anyMatch(snapshotId -> repositoryData.getSnapshotType(snapshotId) == SnapshotType.SHALLOW_COPY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use SnapshotInfo instead to determine if snapshot is a shallow one ?

Copy link
Contributor Author

@harishbhakuni harishbhakuni Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

that would be more costly as we have to read one file for each deleting snapshot in that case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right , looks like it is not that expensive as well . The benefit of this approach is that this will work for existing clusters as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this approach will also work on existing clusters as we will be backfilling this data on existing RepositoryData as well as part of the first RepositoryData update once this change is deployed.

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Aug 1, 2024
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbbafna gbbafna gbbafna left review comments

@anasalkouz anasalkouz Awaiting requested review from anasalkouz anasalkouz is a code owner

@andrross andrross Awaiting requested review from andrross andrross is a code owner

@Bukhtawar Bukhtawar Awaiting requested review from Bukhtawar Bukhtawar is a code owner

@CEHENKLE CEHENKLE Awaiting requested review from CEHENKLE CEHENKLE is a code owner

@dblock dblock Awaiting requested review from dblock dblock is a code owner

@dbwiddis dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner

@dreamer-89 dreamer-89 Awaiting requested review from dreamer-89

@kotwanikunal kotwanikunal Awaiting requested review from kotwanikunal kotwanikunal is a code owner

@mch2 mch2 Awaiting requested review from mch2 mch2 is a code owner

@msfroh msfroh Awaiting requested review from msfroh msfroh is a code owner

@nknize nknize Awaiting requested review from nknize nknize is a code owner

@owaiskazi19 owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 is a code owner

@reta reta Awaiting requested review from reta reta is a code owner

@Rishikesh1159 Rishikesh1159 Awaiting requested review from Rishikesh1159 Rishikesh1159 is a code owner

@sachinpkale sachinpkale Awaiting requested review from sachinpkale sachinpkale is a code owner

@saratvemulapalli saratvemulapalli Awaiting requested review from saratvemulapalli saratvemulapalli is a code owner

@shwetathareja shwetathareja Awaiting requested review from shwetathareja shwetathareja is a code owner

@sohami sohami Awaiting requested review from sohami sohami is a code owner

@tlfeng tlfeng Awaiting requested review from tlfeng

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working skip-changelog stalled Issues that have stalled Storage:Durability Issues and PRs related to the durability framework
Projects
Storage Project Board
Status: Next (Next Quarter)
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Snapshot Interop] Add Shallow Copy Snapshots list in Snapshot Repository Data
2 participants
@harishbhakuni @gbbafna