-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Bugfix] Remote translog does not honour pinned timestamp for low value of indexSettings().getRemoteTranslogExtraKeep() #16078
Merged
gbbafna
merged 1 commit into
opensearch-project:main
from
sachinpkale:bugfix-remotefstranslog
Sep 30, 2024
Merged
[Bugfix] Remote translog does not honour pinned timestamp for low value of indexSettings().getRemoteTranslogExtraKeep() #16078
gbbafna
merged 1 commit into
opensearch-project:main
from
sachinpkale:bugfix-remotefstranslog
Sep 30, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Sachin Kale <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16078 +/- ##
============================================
- Coverage 71.98% 71.97% -0.01%
+ Complexity 64431 64378 -53
============================================
Files 5281 5281
Lines 301063 301067 +4
Branches 43491 43492 +1
============================================
- Hits 216715 216705 -10
- Misses 66526 66531 +5
- Partials 17822 17831 +9 ☔ View full report in Codecov by Sentry. |
sachinpkale
changed the title
Bugfix in RemoteFsTimestampAwareTranslog.trimUnreferencedReaders
[Bugfix] Remote translog does not honour pinned timestamp for low value of indexSettings().getRemoteTranslogExtraKeep()
Sep 25, 2024
sachinpkale
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah,
jainankitk and
linuxpi
as code owners
September 25, 2024 14:12
gbbafna
reviewed
Sep 26, 2024
server/src/internalClusterTest/java/org/opensearch/snapshots/DeleteSnapshotV2IT.java
Show resolved
Hide resolved
linuxpi
approved these changes
Sep 26, 2024
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Sep 30, 2024
) Signed-off-by: Sachin Kale <[email protected]> (cherry picked from commit 1bddf2f) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
gbbafna
pushed a commit
that referenced
this pull request
Sep 30, 2024
) (#16126) (cherry picked from commit 1bddf2f) 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>
hainenber
pushed a commit
to hainenber/OpenSearch
that referenced
this pull request
Oct 1, 2024
…nsearch-project#16078) Signed-off-by: Sachin Kale <[email protected]>
ruai0511
pushed a commit
to ruai0511/OpenSearch
that referenced
this pull request
Oct 4, 2024
…nsearch-project#16078) Signed-off-by: Sachin Kale <[email protected]>
dk2k
pushed a commit
to dk2k/OpenSearch
that referenced
this pull request
Oct 16, 2024
…nsearch-project#16078) Signed-off-by: Sachin Kale <[email protected]>
dk2k
pushed a commit
to dk2k/OpenSearch
that referenced
this pull request
Oct 17, 2024
…nsearch-project#16078) Signed-off-by: Sachin Kale <[email protected]>
dk2k
pushed a commit
to dk2k/OpenSearch
that referenced
this pull request
Oct 21, 2024
…nsearch-project#16078) Signed-off-by: Sachin Kale <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
RemoteFsTimestampAwareTranslog
has different logic fortrimUnreferencedReaders()
when compared to that ofRemoteFsTranslog
. It considers pinned timestamps and makes sure data against pinned timestamp is not deleted from remote translog.RemoteFsTimestampAwareTranslog.trimUnreferencedReaders()
callssuper.trimUnreferencedReaders()
with the intention of cleaning up local translog files.RemoteFsTimestampAwareTranslog
extendsRemoteFsTranslog
, this call to super goes to RemoteFsTranslog which is not aware of pinned timestamps.Translog.trimUnreferencedReaders()
fromRemoteFsTimestampAwareTranslog
.Why is it not failing always?
RemoteFsTimestampAwareTranslog
is inadvertently callingRemoteFsTranslog.trimUnreferencedReaders
the super method becomes a no-op in most of the cases.RemoteFsTranslog.trimUnreferencedReaders
OpenSearch/server/src/main/java/org/opensearch/index/translog/RemoteFsTranslog.java
Lines 575 to 590 in f1acc7a
indexSettings().getRemoteTranslogExtraKeep()
generations always. So, initial calls totrimUnreferencedReaders
would be a no-op.RemoteFsTranslog.trimUnreferencedReaders
, RemoteFsTimestampAwareTranslog removes entries for translog files from fileTracker that are no longer present in the local.RemoteFsTranslog.trimUnreferencedReaders
becomes a no-op and breaks out of the loop.indexSettings().getRemoteTranslogExtraKeep()
is set to low value.Related Issues
Check List
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.