Skip to content
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

[wip] Revert "Disable recycle_log_file_num when it is incompatible with recovery mode (#6351)" #7252

Closed

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Aug 13, 2020

We don't know what the reverted PR solves but suspect it had something to do with 2PC. However, that PR disabled WAL recycling in general scenarios not specific to 2PC, which caused confusion. If we figure out the 2PC problem, it needs to be solved in a more targeted way.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ajkr
Copy link
Contributor Author

ajkr commented Aug 13, 2020

Experimented with it a bit. This behavior doesn't look good:

  1. Create two WALs containing unflushed data for default CF.
In [1]: import rocksdb

In [2]: db = rocksdb.DB('./test-db', rocksdb.Options(create_if_missing=True, wal_recovery_mode='kPointInTimeRecovery', recycle_log_file_num=2))

In [3]: db.create_column_family('a')
Out[3]: 'a'

In [4]: db.put(b'ok', b'ok', 'a')  # PUT("ok", "ok") in CF a

In [5]: db.put(b'key1', b'wal1')  # PUT("key1", "wal1") in CF default 

In [6]: db.flush('a')  # Flush CF a to trigger a new WAL to be cut. Now there are two unflushed WALs for CF default

In [7]: db.put(b'key2', b'wal2')  # PUT("key2", "wal2") in CF default

In [8]: del db
  1. Manually corrupt the second entry in the first WAL (PUT("key1", "wal1"))

  2. Reopen the DB. Observe recovery permitted a hole, which is not a consistent point in time.

In [3]: db = rocksdb.DB('./test-db', rocksdb.Options(create_if_missing=True, wal_recovery_mode='kPointInTimeRecovery', recycle_log_file_num=2), column_families=['default', 'a'])

In [4]: db.get(b'key1'). # DB contains nothing for key1

In [5]: db.get(b'key2'). # Meanwhile, it contains the value for "key2", which is supposed to be newer!
Out[5]: b'wal2'

@ajkr ajkr changed the title Revert "Disable recycle_log_file_num when it is incompatible with recovery mode (#6351)" [wip] Revert "Disable recycle_log_file_num when it is incompatible with recovery mode (#6351)" Aug 14, 2020
@ajkr
Copy link
Contributor Author

ajkr commented Aug 14, 2020

Well, the exact same problem still happens with kTolerateCorruptedTailRecords, which we have yet to mark incompatible with WAL recycling. Arguably that one is worse as it's supposed to give a stronger recovery guarantee. Anyways, the plan is now to enable it in kPointInTimeRecovery and disable it in kTolerateCorruptedTailRecords. To do this, we first need to make a checksum mismatch error actually stop the WAL replay, even if newer WALs are present.

@ajkr
Copy link
Contributor Author

ajkr commented Oct 16, 2020

No longer WIP (at least not by me). For the short term fix for the problem above, #7271 disabled the feature more broadly.

@ajkr ajkr closed this Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants