[Remote Translog] Deleting remote translog considering latest remote metadata #5869
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
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #5869 +/- ##
============================================
+ Coverage 70.88% 70.91% +0.02%
- Complexity 58720 58776 +56
============================================
Files 4768 4768
Lines 280575 280584 +9
Branches 40514 40516 +2
============================================
+ Hits 198881 198971 +90
+ Misses 65334 65327 -7
+ Partials 16360 16286 -74
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
gbbafna
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
adnapibar,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
ashking94
reviewed
Jan 16, 2023
server/src/main/java/org/opensearch/index/translog/RemoteFsTranslog.java
Show resolved
Hide resolved
ashking94
reviewed
Jan 16, 2023
server/src/main/java/org/opensearch/index/translog/RemoteFsTranslog.java
Outdated
Show resolved
Hide resolved
ashking94
reviewed
Jan 16, 2023
| } | ||
| } | ||
|
|
||
| public void trimUnreferencedReaders() throws IOException { |
ashking94
reviewed
Jan 16, 2023
| logger.trace("delete remote translog generation file [{}], not referenced by metadata anymore", generation); | ||
| deleteRemoteGeneration(generation); | ||
| } else { | ||
| break; |
There was a problem hiding this comment.
Why are we breaking out of the for loop?
I can think of that we may want to retry the uploads - in which case should we start the for loop from minimum generation number that exists?
There was a problem hiding this comment.
this would increase the flush times to a very high number. The clean up can be taken as an async job on its own .
ashking94
reviewed
Jan 16, 2023
| String translogFilename = Translog.getFilename(generation); | ||
| if (fileTransferTracker.uploaded(translogFilename)) { | ||
| logger.trace("delete remote translog generation file [{}], not referenced by metadata anymore", generation); | ||
| deleteRemoteGeneration(generation); |
There was a problem hiding this comment.
deleteRemoteGeneration method uses the current primaryTerm. This probably would not be handled when there is failover and the primary term has increased. Could we rely on metadata to fetch the right generation to primary term mapping for cleaning up the files?
There was a problem hiding this comment.
That's a great catch @ashking94 . The issue here is the latest metadata doesn't know about the right primary term for this generation. I will create a backlog item for cleaning up the older primary term translog files . This can be done on every failover or an async job . I don't want to complicate the usual deletion flow due to this corner case .
gbbafna
changed the title
gbbafna
changed the title
gbbafna
force-pushed
the
tx-safe-remote-delete
branch
from
9be02e5 to
246924d
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
gbbafna
force-pushed
the
tx-safe-remote-delete
branch
from
2422c10 to
950c2bc
Compare
Gradle Check (Jenkins) Run Completed with:
|
ashking94
added
Storage:Durability
Signed-off-by: Gaurav Bafna <[email protected]>
Signed-off-by: Gaurav Bafna <[email protected]>
Signed-off-by: Gaurav Bafna <[email protected]>
Signed-off-by: Gaurav Bafna <[email protected]>
Signed-off-by: Ashish Singh <[email protected]>
ashking94
force-pushed
the
tx-safe-remote-delete
branch
from
950c2bc to
61344d4
Compare
Gradle Check (Jenkins) Run Completed with:
|
|
Created PR #6086 for rest of the development due to access issue. |
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.
Signed-off-by: Gaurav Bafna [email protected]
Description
We should delete remote translog considering the latest metadata file uploaded. The tlog/ckp files referenced by that metadata file cannot be deleted . By doing this we will be able to restore translog from that metadata.
Issues Resolved
#5845
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.