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

Add timestamp pinning service and scheduler to update in-memory state #15180

Merged

sachinpkale merged 12 commits into opensearch-project:main from sachinpkale:timestamp-pinning

Aug 15, 2024

Member

sachinpkale commented Aug 9, 2024 •

edited

Loading

Description

As part of timestamp pinning , we are adding support of pinning the timestamp for remote backed indices.
In this PR, we introduce a service which provides APIs to pin and unpin the timestamp.
Once the pinned timestamp list is updated, we need to modify in-memory data structure at each node. This is done by a scheduler which runs periodically and updates the local state.
The periodicity of this scheduler is controlled by a dynamic cluster setting.

Related Issues

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.

sachinpkale force-pushed the timestamp-pinning branch 2 times, most recently from 1179bfd to 83c945b Compare

August 9, 2024 10:14

Contributor

github-actions bot commented Aug 9, 2024

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

Contributor

github-actions bot commented Aug 9, 2024

❌ Gradle check result for 1179bfd: 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 force-pushed the timestamp-pinning branch from 83c945b to 3fe94b0 Compare

August 9, 2024 11:11

Contributor

github-actions bot commented Aug 9, 2024

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

Contributor

github-actions bot commented Aug 9, 2024

❌ Gradle check result for 3fe94b0: 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 commented

View reviewed changes

server/src/main/java/org/opensearch/node/Node.java Show resolved Hide resolved

sachinpkale commented

View reviewed changes

.../src/main/java/org/opensearch/gateway/remote/model/RemoteStorePinnedTimestampsBlobStore.java Outdated Show resolved Hide resolved

sachinpkale force-pushed the timestamp-pinning branch from 4075aa1 to 9b2823c Compare

August 9, 2024 13:48

sachinpkale added the skip-changelog label

sachinpkale marked this pull request as ready for review

August 9, 2024 13:49

sachinpkale requested review from anasalkouz, andrross, ashking94, Bukhtawar, CEHENKLE, dblock, dbwiddis, gbbafna, kotwanikunal, linuxpi, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, saratvemulapalli and shwetathareja as code owners

August 9, 2024 13:49


          Fix javadocs

3bad790

Signed-off-by: Sachin Kale <[email protected]>

Contributor

github-actions bot commented Aug 14, 2024

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

Contributor

github-actions bot commented Aug 14, 2024

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

linuxpi reviewed

View reviewed changes

server/src/main/java/org/opensearch/node/remotestore/RemoteStorePinnedTimestampService.java Show resolved Hide resolved

server/src/main/java/org/opensearch/node/remotestore/RemoteStorePinnedTimestampService.java Outdated Show resolved Hide resolved

server/src/main/java/org/opensearch/node/remotestore/RemoteStorePinnedTimestampService.java Show resolved Hide resolved

server/src/main/java/org/opensearch/node/remotestore/RemoteStorePinnedTimestampService.java Outdated Show resolved Hide resolved

server/src/main/java/org/opensearch/gateway/remote/model/RemotePinnedTimestamps.java Show resolved Hide resolved

server/src/main/java/org/opensearch/node/remotestore/RemoteStorePinnedTimestampService.java Show resolved Hide resolved

linuxpi reviewed

View reviewed changes

server/src/main/java/org/opensearch/node/remotestore/RemoteStorePinnedTimestampService.java Show resolved Hide resolved

Contributor

github-actions bot commented Aug 14, 2024

❕ Gradle check result for 3bad790: UNSTABLE

TEST FAILURES:

      1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationMultiSearchDuringQueryPhase

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

gbbafna approved these changes

View reviewed changes

codecov bot commented Aug 14, 2024 •

edited

Loading

Codecov Report

Attention: Patch coverage is 46.95122% with 87 lines in your changes missing coverage. Please review.

Project coverage is 71.83%. Comparing base (6e27e5a) to head (4212363).
Report is 80 commits behind head on main.

Files with missing lines	Patch %	Lines
...remotestore/RemoteStorePinnedTimestampService.java	31.89%	77 Missing and 2 partials ⚠️
...h/gateway/remote/model/RemotePinnedTimestamps.java	78.78%	5 Missing and 2 partials ⚠️
...te/model/RemoteStorePinnedTimestampsBlobStore.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #15180      +/-   ##
============================================
- Coverage     71.89%   71.83%   -0.07%     
- Complexity    63043    63064      +21     
============================================
  Files          5197     5200       +3     
  Lines        295311   295474     +163     
  Branches      42676    42687      +11     
============================================
- Hits         212314   212246      -68     
- Misses        65552    65827     +275     
+ Partials      17445    17401      -44

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


          Address PR comments

Signed-off-by: Sachin Kale <[email protected]>

sachinpkale mentioned this pull request

Make number of pinned_timestamp files to keep in remote configurable. #15248

Closed

Contributor

github-actions bot commented Aug 14, 2024

✅ Gradle check result for 4212363: SUCCESS

Member Author

sachinpkale commented Aug 15, 2024

Codecov patch is failing as integ test coverage is not covered (added in RemoteStorePinnedTimestampsIT)

sachinpkale merged commit 1717b55 into opensearch-project:main

34 of 35 checks passed

sachinpkale added the backport 2.x label

Contributor

opensearch-trigger-bot bot commented Aug 15, 2024

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-15180-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1717b551db0d6e5d22aff89bf6fd9873d8d4ec20
# Push it to GitHub
git push --set-upstream origin backport/backport-15180-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-15180-to-2.x.

opensearch-trigger-bot bot added the backport-failed label

soosinha reviewed

View reviewed changes

server/src/main/java/org/opensearch/node/Node.java

@@ @@ -807,6 +808,18 @@ protected Node( @@
                               remoteIndexPathUploader = null;
                               remoteClusterStateCleanupManager = null;
                           }
+                          final RemoteStorePinnedTimestampService remoteStorePinnedTimestampService;
+                          if (isRemoteStoreAttributePresent(settings)) {

Member

soosinha Aug 16, 2024

Should use isRemoteDataAttributePresent here instead since this is required for remote data store.

linuxpi reviewed

View reviewed changes

server/src/main/java/org/opensearch/node/remotestore/RemoteStorePinnedTimestampService.java

+                      BlobPath path = pinnedTimestampsBlobStore.getBlobPathForUpload(remotePinnedTimestamps);
+                      try {
+                          if (updateTimetampPinningSemaphore.tryAcquire(10, TimeUnit.MINUTES)) {
+                              ActionListener<Void> semaphoreAwareListener = ActionListener.runBefore(listener, updateTimetampPinningSemaphore::release);

Collaborator

linuxpi Aug 20, 2024

any unhandled exception is not logged in the listener chaining, we should log it to get visibility into unexpected failures

wdongyu pushed a commit to wdongyu/OpenSearch that referenced this pull request


          Add timestamp pinning service and scheduler to update in-memory state (…

…opensearch-project#15180)

---------

Signed-off-by: Sachin Kale <[email protected]>
Co-authored-by: Sachin Kale <[email protected]>

sachinpkale mentioned this pull request

Change pinned timestamp file format stored in remote store #15526

Merged

3 tasks

sachinpkale added backport 2.x and removed backport 2.x labels

opensearch-trigger-bot bot pushed a commit that referenced this pull request


          Add timestamp pinning service and scheduler to update in-memory state (…

7cd1ed9

…#15180)

---------

Signed-off-by: Sachin Kale <[email protected]>
Co-authored-by: Sachin Kale <[email protected]>
(cherry picked from commit 1717b55)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

opensearch-trigger-bot bot mentioned this pull request

[Backport 2.x] Add timestamp pinning service and scheduler to update in-memory state #15547

Merged

sachinpkale pushed a commit that referenced this pull request


          Add timestamp pinning service and scheduler to update in-memory state (…

10b955d

…#15180) (#15547)

---------



(cherry picked from commit 1717b55)

Signed-off-by: Sachin Kale <[email protected]>
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>
Co-authored-by: Sachin Kale <[email protected]>

akolarkunnu pushed a commit to akolarkunnu/OpenSearch that referenced this pull request


          Add timestamp pinning service and scheduler to update in-memory state (…

f7f9071

…opensearch-project#15180)

---------

Signed-off-by: Sachin Kale <[email protected]>
Co-authored-by: Sachin Kale <[email protected]>

This was referenced Sep 11, 2024

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStoreStatsIT #14310

Open

[AUTOCUT] Gradle Check Flaky Test Report for SearchRestCancellationIT #14311

Open

[AUTOCUT] Gradle Check Flaky Test Report for AzureBlobContainerRetriesTests #14318

Open

[AUTOCUT] Gradle Check Flaky Test Report for SegmentReplicationUsingRemoteStoreIT #14329

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

linuxpi linuxpi left review comments

Bukhtawar Bukhtawar left review comments

soosinha soosinha left review comments

gbbafna gbbafna approved these changes

anasalkouz Awaiting requested review from anasalkouz anasalkouz is a code owner

andrross Awaiting requested review from andrross andrross is a code owner

ashking94 Awaiting requested review from ashking94 ashking94 is a code owner

CEHENKLE Awaiting requested review from CEHENKLE CEHENKLE is a code owner

dblock Awaiting requested review from dblock dblock is a code owner

dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner

kotwanikunal Awaiting requested review from kotwanikunal kotwanikunal is a code owner

mch2 Awaiting requested review from mch2 mch2 is a code owner

msfroh Awaiting requested review from msfroh msfroh is a code owner

nknize Awaiting requested review from nknize nknize is a code owner

owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 is a code owner

reta Awaiting requested review from reta reta is a code owner

Rishikesh1159 Awaiting requested review from Rishikesh1159 Rishikesh1159 is a code owner

saratvemulapalli Awaiting requested review from saratvemulapalli saratvemulapalli is a code owner

shwetathareja Awaiting requested review from shwetathareja shwetathareja is a code owner

sohami Awaiting requested review from sohami sohami is a code owner

VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

Labels

backport 2.x backport-failed skip-changelog