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

Optimize remote store GC flow with pinned timestamps #15943

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Optimize remote store GC flow with pinned timestamps #15943

wants to merge 8 commits into from

Conversation

sachinpkale
Copy link
Member

@sachinpkale sachinpkale commented Sep 16, 2024 •
edited
Loading

Description

  • Added a bunch of optimisations in remote store garbage collection flow to work with pinned timestamps
    • If index is deleted and a snapshot containing the index is getting deleted:
      • If this is the last snapshot containing the index, delete remote store data (segments + translog)
      • else pass pinning entity for the snapshot that is getting deleted. This is required to delete the data specific to snapshot even if pinned timestamp scheduler runs later.
    • For remote translog, avoid calling list metadata files if minRemoteGenReferenced - indexSettings().getRemoteTranslogExtraKeep() <= maxDeletedGenerationOnRemote
    • For remote translog, make sure to keep metadata and generation deletion in sync. With this, if metadata file is present, we can ensure that generations corresponding to it are not deleted.
    • Added integ tests around these scenarios.

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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
Copy link
Contributor

❌ Gradle check result for 609bd01: 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?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 3186ffa: 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?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for f07d64d: 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?

@opensearch-ci-bot opensearch-ci-bot mentioned this pull request Sep 16, 2024
Open
@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for c344045: 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?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for eb12d3c: 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?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 6a4dab7: 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?

This was referenced Sep 17, 2024
Open
Open
Open
Open
Open
@sachinpkale sachinpkale changed the title Optimize GC flow with pinned timestamps Optimize remote store GC flow with pinned timestamps Sep 18, 2024
@sachinpkale sachinpkale marked this pull request as ready for review September 18, 2024 03:36
@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 22187b9: 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?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 36eba58: 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?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 36eba58: 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?

@github-actions GitHub Actions
Copy link
Contributor

✅ Gradle check result for fae9f37: SUCCESS

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 04f2a7d: 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?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for f649a25: 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?

@sachinpkale
Copy link
Member Author

Failure in https://build.ci.opensearch.org/job/gradle-check/48328/ seems related to pinned timestamps. Taking a look

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for f649a25: 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?

This was referenced Sep 24, 2024
Open
Open
@sachinpkale
Fix flaky test
c53643c 
Signed-off-by: Sachin Kale <[email protected]>
gbbafna
gbbafna reviewed Sep 24, 2024
View reviewed changes
server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java
if (indexDeleted == false && previousMinRemoteGenReferenced == minRemoteGenReferenced) {
return;
} else if (previousMinRemoteGenReferenced != minRemoteGenReferenced) {
previousMinRemoteGenReferenced = minRemoteGenReferenced;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we update previousMinRemoteGenReferenced when we update minRemoteGenReferenced , as it looks the correct place for same ?

server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java
@@ -61,6 +61,7 @@ public class RemoteFsTimestampAwareTranslog extends RemoteFsTranslog {
private final Map<String, Tuple<Long, Long>> oldFormatMetadataFileGenerationMap;
private final Map<String, Tuple<Long, Long>> oldFormatMetadataFilePrimaryTermMap;
private final AtomicLong minPrimaryTermInRemote = new AtomicLong(Long.MAX_VALUE);
private long previousMinRemoteGenReferenced = -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we move this to RemoteFSTranslog as it doesn't look related to Timestamp Aware Translog ?

...sterTest/java/org/opensearch/remotestore/RemoteStorePinnedTimestampsGarbageCollectionIT.java
.build();
}

private void keepPinnedTimestampSchedulerUpdated() throws InterruptedException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

would this fail when pinned timestamp service is running at > 10 sec periodicity ? Should the sleep account for it ?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for c53643c: 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?

This was referenced Sep 24, 2024
Open
Open
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@linuxpi linuxpi linuxpi left review comments

@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

@ashking94 ashking94 Awaiting requested review from ashking94 ashking94 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

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

@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

@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

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

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

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

Assignees
No one assigned
Labels
skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sachinpkale @linuxpi @gbbafna