Add timestamp support in dump_wal/dump/idump #12690
Conversation
| @@ -2262,7 +2293,7 @@ void DBDumperCommand::DoDumpCommand() { | |||
| for (; iter->Valid(); iter->Next()) { | |||
| int rawtime = 0; | |||
| // If end marker was specified, we stop before it | |||
| if (!null_to_ && (iter->key().ToString() >= to_)) { | |||
| if (!null_to_ && ucmp->Compare(iter->key(), to_) >= 0) { | |||
tools/ldb_cmd.cc
Outdated
| if (ts_sz == 0) { | ||
| return LDBCommand::StringToHex(key.ToString()); | ||
| } else { | ||
| assert(key.size() >= ts_sz); |
There was a problem hiding this comment.
I think this assertion might not hold in all cases (e.g. corruption or undetected comparator change), right? Maybe we could print something like "CORRUPT KEY" instead if the key is too short
There was a problem hiding this comment.
Thank you for the suggestion! I have updated this.
| @@ -2166,7 +2189,7 @@ void DBDumperCommand::DoCommand() { | |||
| // TODO(myabandeh): allow configuring is_write_commited | |||
| DumpWalFile(options_, path_, /* print_header_ */ true, | |||
| /* print_values_ */ true, true /* is_write_commited */, | |||
| &exec_state_); | |||
| ucmps_, &exec_state_); | |||
There was a problem hiding this comment.
One general question here: we now allow changing the comparator e.g. to enable UDTS on an existing CF, so the current comparator might be different from what was in effect when a given WAL record was written, right? If so, would it make sense to have some sanity checks around timestamp sizes (using the TS size info from the WAL) before using the current comparator? (This only affects dump_wal I think; for dump and idump, recovered KVs would go through the timestamp reconciliation logic.)
There was a problem hiding this comment.
This is a good point! I have introduced using the HandleWriteBatchTimestampSizeDifference util function with kVerifyConsistency mode when DB is opened and running comparators are provided. To check any user key format discrepancy between the comparator's expectation and WAL file's recorded timestamp size.
The current handling is we don't proceed if any discrepancy is detected and message like this will be printed:
Failed: Format for user keys in WAL file is inconsistent with the comparator used to open the DB. Timestamp size recorded in WAL vs specified by comparator: {(cf_id: 0, [recorded: 8, comparator: 0])}
I found that this PR's change introduced usage of ColumnFamilyCollector and TimestampRecoveryHandler for ldb's wal dump path. And those two were missing override implementations for both PutEntityCF and TimedPutCF. So I added those in this PR too.
There was a problem hiding this comment.
To quote Ron Burgundy, "that escalated quickly"! Thanks so much for adding these
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jowlyzhang merged this pull request in 9a72cf1. |
Summary: As titled. For dumping wal files, since a mapping from column family id to the user comparator object is needed to print the timestamp in human readable format, option `[--db=<db_path>]` is added to `dump_wal` command to allow the user to choose to optionally open the DB as read only instance and dump the wal file with better timestamp formatting. Pull Request resolved: facebook#12690 Test Plan: Manually tested dump_wal: [dump a wal file specified with --walfile] ``` >> ./ldb --walfile=$TEST_DB/000004.log dump_wal --print_value >>1,1,28,13,PUT(0) : 0x666F6F0100000000000000 : 0x7631 (Column family id: [0] contained in WAL are not opened in DB. Applied default hex formatting for user key. Specify --db=<db_path> to open DB for better user key formatting if it contains timestamp.) ``` [dump with --db specified for better timestamp formatting] ``` >> ./ldb --walfile=$TEST_DB/000004.log dump_wal --db=$TEST_DB --print_value >> 1,1,28,13,PUT(0) : 0x666F6F|timestamp:1 : 0x7631 ``` dump: [dump a file specified with --path] ``` >>./ldb --path=/tmp/rocksdbtest-501/column_family_test_75359_17910784957761284041/000004.log dump Sequence,Count,ByteSize,Physical Offset,Key(s) : value 1,1,28,13,PUT(0) : 0x666F6F0100000000000000 : 0x7631 (Column family id: [0] contained in WAL are not opened in DB. Applied default hex formatting for user key. Specify --db=<db_path> to open DB for better user key formatting if it contains timestamp.) ``` [dump db specified with --db] ``` >> ./ldb --db=/tmp/rocksdbtest-501/column_family_test_75359_17910784957761284041 dump >> foo|timestamp:1 ==> v1 Keys in range: 1 ``` idump ``` ./ldb --db=$TEST_DB idump 'foo|timestamp:1' seq:1, type:1 => v1 Internal keys in range: 1 ``` Reviewed By: ltamasi Differential Revision: D57755382 Pulled By: jowlyzhang fbshipit-source-id: a0a2ef80c92801cbf7bfccc64769c1191824362e
As titled. For dumping wal files, since a mapping from column family id to the user comparator object is needed to print the timestamp in human readable format, option
[--db=<db_path>]is added todump_walcommand to allow the user to choose to optionally open the DB as read only instance and dump the wal file with better timestamp formatting.Test plan:
Manually tested
dump_wal:
[dump a wal file specified with --walfile]
[dump with --db specified for better timestamp formatting]
dump:
[dump a file specified with --path]
[dump db specified with --db]
idump