-
Notifications
You must be signed in to change notification settings - Fork 24
add rollback on state machine add open Leader_Restart ut #585
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
Conversation
JacksonYao287
left a comment
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.
rollback_config should be paid more attention
xiaoxichen
left a comment
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.
lgtm.
Also good to see we enabled one more UT.
082c1bf to
4737b66
Compare
Previously commit dont flush the over-written log, and the log_entries only look at flushed logs. As a result, the over-written log was not included in the log_entries() result which breaks the UT. The UT was running in master, because we dont at the over_written entry into cache and the entry_at() call will inline flush this entry. After adding it to cache it served from cache without flushing. Signed-off-by: Xiaoxi Chen <xiaoxchen@ebay.com>
|
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 #585 +/- ##
===========================================
+ Coverage 56.51% 67.22% +10.71%
===========================================
Files 108 109 +1
Lines 10300 10713 +413
Branches 1402 1463 +61
===========================================
+ Hits 5821 7202 +1381
+ Misses 3894 2816 -1078
- Partials 585 695 +110 ☔ View full report in Codecov by Sentry. |
6420bec to
d10b519
Compare
| @@ -152,6 +152,7 @@ TEST_F(RaftReplDevTest, Resync_From_Non_Originator) { | |||
| } | |||
|
|
|||
| #if 0 | |||
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.
Can this test case be enabled or not yet?
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.
@Besroy had tried to enable this but failed. The test of restart leader is passing in Storage hammer but not here.. @JacksonYao287 traiged this, pls share more findings?
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.
As far as I know, the UT's write_on_leader function cannot detect changes in the leader. If a leader switch occurs while writing data, this UT will get stuck. Although it works locally, it may fail occasionally in Jenkins, which is why it has been commented out again. @JacksonYao287 Please correct me if I'm wrong.
* add rollback on state machine --------- Signed-off-by: yawzhang <yawzhang@ebay.com>
#582