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
Introduce translog size and age based retention policies #25147
Changes from 13 commits
3e32b7a
5f01ce6
7da706c
da56a0d
9d0cf47
a520407
659fb6c
08a2b7a
88d7a32
3b62254
1a638cf
f1dd56f
a3c4f2f
384b795
5d97182
10ac31a
923152d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,7 +116,7 @@ public static TranslogReader open( | |
throw new IllegalStateException("pre-2.0 translog found [" + path + "]"); | ||
case TranslogWriter.VERSION_CHECKPOINTS: | ||
assert path.getFileName().toString().endsWith(Translog.TRANSLOG_FILE_SUFFIX) : "new file ends with old suffix: " + path; | ||
assert checkpoint.numOps >= 0 : "expected at least 0 operatin but got: " + checkpoint.numOps; | ||
assert checkpoint.numOps >= 0 : "expected at least 0 operation but got: " + checkpoint.numOps; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just an idea, should we use the last modification time on the file instead? I mean it should be ok to do that no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tja, maybe. I tried to avoid the entire what metadata is supported where discussion. I mean we walk to great length to avoid directory traversals. I think this is simple, solid and entirely self contained? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I agree with this. it's redundant information and no traversal needed. you have the file you just need to read the time created. I am really careful with any kind of added metadata here you know why. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I totally hear you. It's clearly and edge case. One other aspect of they way I set it up now is that it's easy to test. With FS based metadata we'd probably have to mock something, which is also doable but messier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I lean towards using the filesystem mtime here. I do not think this would be messy to mock, only a method that you can override in tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. I personally feel it might end up messier but I trust your judgement. I'll roll back the extra inlined creation date and move to using FS mtime. We can always re-add the inline creation date if we run into trouble. Do-The-Minimal-Change-Needed™️ |
||
assert checkpoint.offset <= channel.size() : "checkpoint is inconsistent with channel length: " + channel.size() + " " + checkpoint; | ||
int len = headerStream.readInt(); | ||
if (len > channel.size()) { | ||
|
@@ -130,8 +130,8 @@ public static TranslogReader open( | |
throw new TranslogCorruptedException("expected shard UUID " + uuidBytes + " but got: " + ref + | ||
" this translog file belongs to a different translog. path:" + path); | ||
} | ||
final long firstOperationOffset = | ||
ref.length + CodecUtil.headerLength(TranslogWriter.TRANSLOG_CODEC) + Integer.BYTES; | ||
final long firstOperationOffset; | ||
firstOperationOffset = ref.length + CodecUtil.headerLength(TranslogWriter.TRANSLOG_CODEC) + Integer.BYTES; | ||
return new TranslogReader(checkpoint, channel, path, firstOperationOffset); | ||
|
||
default: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no
max
on this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question. Will add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will actually remove the max from the other one. Shorter.