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

Change pinned timestamp file format stored in remote store #15526

Merged

Conversation

sachinpkale
Copy link
Member

@sachinpkale sachinpkale commented Aug 30, 2024

Description

  • As part of Add timestamp pinning service and scheduler to update in-memory state #15180, we introduced a way to pin/unpin timestamp and stored the end state in the configured remote store.
  • But storing all the pinned timestamps in a file can have concurrency issues even if we create immutable files to store the info.
  • While debugging, reading these pinned timestamp files in remote is also not always possible if the content is encrypted.
  • To tackle these problems, in this PR, we change the way to store pinned timestamp data. Instead of storing all the timestamps in one file, we create separate, empty file per <timestamp, pinning entity> tuple.
  • We name the file as <pinning entity>__<timestamp> which makes debugging easier just by listing the files.

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.

Copy link
Contributor

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

Copy link
Contributor

✅ Gradle check result for 5c4dcf9: SUCCESS

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 28.57143% with 45 lines in your changes missing coverage. Please review.

Project coverage is 71.90%. Comparing base (738cdd3) to head (38c36b9).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...remotestore/RemoteStorePinnedTimestampService.java 25.00% 43 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main   #15526   +/-   ##
=========================================
  Coverage     71.89%   71.90%           
- Complexity    63791    63832   +41     
=========================================
  Files          5249     5247    -2     
  Lines        298149   298109   -40     
  Branches      43084    43084           
=========================================
- Hits         214368   214354   -14     
- Misses        66126    66199   +73     
+ Partials      17655    17556   -99     

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

Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Sep 2, 2024

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

Copy link
Contributor

github-actions bot commented Sep 2, 2024

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

Sachin Kale added 5 commits September 2, 2024 23:20
Copy link
Contributor

github-actions bot commented Sep 2, 2024

❕ Gradle check result for 38c36b9: UNSTABLE

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

@sachinpkale
Copy link
Member Author

Changes are mostly covered in the integ tests. CodeCov is failing as it does not consider coverage from integ tests.

@sachinpkale sachinpkale merged commit cfcfe21 into opensearch-project:main Sep 3, 2024
33 of 34 checks passed
@sachinpkale sachinpkale added the backport 2.x Backport to 2.x branch label Sep 3, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 3, 2024
---------

Signed-off-by: Sachin Kale <[email protected]>
Co-authored-by: Sachin Kale <[email protected]>
(cherry picked from commit cfcfe21)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sachinpkale pushed a commit that referenced this pull request Sep 3, 2024
…15587)

---------

(cherry picked from commit cfcfe21)

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 Sep 10, 2024
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 16, 2024
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 17, 2024
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 skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants