Fix rollback when rollback to tail_lsn. #532
Conversation
previously we dont properly handle this case, which will error out in https://github.com/eBay/HomeStore/blob/2080ad42e31a9ddf0a41d3a112db4d1c020c45ee/src/lib/logstore/log_store.cpp#L315 as the to_lsn + 1 is invalid in this case. Signed-off-by: Xiaoxi Chen <[email protected]>
xiaoxichen
force-pushed
the
fix_rollback
branch
from
9f2c1e1 to
57e9968
Compare
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #532 +/- ##
===========================================
+ Coverage 56.51% 67.53% +11.01%
===========================================
Files 108 109 +1
Lines 10300 10429 +129
Branches 1402 1401 -1
===========================================
+ Hits 5821 7043 +1222
+ Misses 3894 2710 -1184
- Partials 585 676 +91 ☔ View full report in Codecov by Sentry. |
| @@ -166,11 +166,6 @@ class SampleLogStoreClient { | |||
| } | |||
|
|
|||
| void rollback_validate(uint32_t num_lsns_to_rollback) { | |||
|
|
|||
There was a problem hiding this comment.
why this is removed?
There was a problem hiding this comment.
I believe @sanebay hit this issue when he was doing the log store long running... he worked around in the UT but with the fix in implementation we dont need this work around in UT.
src/lib/logstore/log_store.cpp
Outdated
| //Fast path | ||
| if (to_lsn == m_tail_lsn.load()) { | ||
| return true; | ||
| } | ||
|
|
||
| if (to_lsn > m_tail_lsn.load()) { | ||
| HS_LOG_ASSERT(false, "Attempted to rollback to {} which is larger than tail_lsn {}", to_lsn, m_tail_lsn.load()); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
actually , what rollback does is to reset the tail_lsn to a valid lsn in the range [start_lsn , tail_lsn]. so if the rollback_to_lsn is not in the range, we can log an error and return false.
so my suggestion is adding the following code is enough, and the check in line 297 can also be removed.
if(to_lsn < m_start_lsn || to_lsn > m_tail_lsn) {
LOGERROR;
return false;
}
Signed-off-by: Xiaoxi Chen <[email protected]>
xiaoxichen
force-pushed
the
fix_rollback
branch
from
b2dfa16 to
91a4566
Compare
| } | ||
|
|
||
| if (to_lsn > m_tail_lsn.load() || to_lsn < m_start_lsn.load()) { | ||
| HS_LOG_ASSERT(false, "Attempted to rollback to {} which is not in the range of [{}, {}]", to_lsn, m_start_lsn.load(), m_tail_lsn.load()); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: Can we also have an assert here for HS_DEBUG_ASSERT(m_records.status(to_lsn + 1).is_out_of_range == false)?
There was a problem hiding this comment.
there are two angle,
- from api point of view, why we want to assert for a no-op rollback? i. e if last log is 100, rollback to 100 is a valid request
- from the implementation of streamtracker. the is_out_of_range will not be set for LSN > upper_bound.
https://github.com/eBay/sisl/blob/ed773dbb0c7e53f46b29374e477f4be3dc1db99b/include/sisl/fds/stream_tracker.hpp#L128
There was a problem hiding this comment.
the sisl one can be take as a bug, IMO.
previously we dont properly handle this case,
which will error out in
HomeStore/src/lib/logstore/log_store.cpp
Line 315 in 2080ad4