-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
support set global seqno for external file #3068
support set global seqno for external file #3068
Conversation
@ajkr PTAL |
@mikhail-antonov do you mind taking a look? thanks |
} | ||
|
||
// Set the global sequence number | ||
*seqno = DecodeFixed64(seqno_iter->second.c_str()); |
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.
uhm i'm unsure where the actual write happens? missing something obvious? Or at() writes empty string here?
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.
uhm, i guess we only read here, so comment above is a bit misleading (/set the global number)
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.
looks good to me!
@zhangjinpeng1987 do you mind rebasing and fix the compilation issues? |
@zhangjinpeng1987 do we still need this? |
As part of clean-up, can we reset the global seqno once ingestion fails? Adding a new public API seems to be an overkill here. |
After supporting bulkloading without touching the file to load, do we still need this PR? |
If a user still chooses to write global seq, which we support for backward compatibility, we may still need this PR. |
We don't need this feature anymore. And I think @riversand963 already changed the checksum logic to ignore global seq num? |
Yeah, I updated checksum verification logic for properties block of external sst file. Specifically, when you verify the checksum for a properties block of such sst file, RocksDB will also try to compute the checksum assuming the global seqno is 0 and compare it with the checksum stored persistently in the file. This fix can avoid returning Status::Corruption for externally ingested SST files whose global seqno may have been updated. However, it does not change global seqno back to its initial value (0). In contrast, finishing this PR should be able to revert the SST file to its original state completely, which seems cleaner to some extent. |
Before ingesting external file, we usually need to verify the checksum of the external file to guarantee the file is not corrupted.
When ingesting fail before version change has been added to MANIFEST, but after global_seqno has been set with
move_files = true and allow_global_seqno = true
mode(actually use hard link instead of move), if we restart the ingesting process, checksum verify will fail because of the global_seqno has been changed. So we need to reset the global sequence number to zero before verify.