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.

Sign up for GitHub

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

Jump to bottom

Add Statemachine feature for DLedger #133

Merged
merged 2 commits into from
Apr 28, 2022
Merged

Add Statemachine feature for DLedger #133

merged 2 commits into from
Apr 28, 2022

Conversation

RongtongJin
Copy link
Contributor

Add Statemachine feature for DLedger

@hzh0425
Feature: add statemachine for dledger (#128)
4293093 
* feature: add statemachine add statemachine caller

* feature: add fsm caller to dledgerEntryPusher

* feature: Improve the function of the state machine

* style: add apach header

* style: add apache header
@hzh0425
Feature: let dledger complete responseFutures after applying entries …
6775159 
…to statemachine (#132)
@RongtongJin RongtongJin requested a review from yuz10 April 26, 2022 12:52
@RongtongJin
Copy link
Contributor Author

RongtongJin commented Apr 26, 2022 •
edited
Loading

@yuz10 @Git-Yang @cserwen Could you help review this PR? Statemachine feature will not affect normal processes if not use.

@RongtongJin
Copy link
Contributor Author

RongtongJin commented Apr 27, 2022 •
edited
Loading

while (iter.hasNext()) is also called in statemachine.onApply, and why do we complete client response in iter.hasNext(), before the statemachine is really applied

其实这里是complete上一个的，就是已经被apply的

@hzh0425
Copy link
Contributor

hzh0425 commented Apr 27, 2022

while (iter.hasNext()) is also called in statemachine.onApply, and why do we complete c

yep, complete the pre entryResponseFuture

ShannonDing
ShannonDing approved these changes Apr 27, 2022
View reviewed changes
Copy link
Member

@ShannonDing ShannonDing left a comment

Choose a reason for hiding this comment

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

LGTM

@odbozhou odbozhou merged commit 6f0bf36 into master Apr 28, 2022
@cserwen
Copy link
Contributor

cserwen commented Apr 28, 2022

@hzh0425 There are some errors in the unit test.

@hzh0425
Copy link
Contributor

hzh0425 commented Apr 28, 2022

@cserwen hi, i have fixed the bug in #139,
except (DLedgerMappedFileStoreTest # testAbnormalRecovery ), it can't even run successfully in the old version

@cserwen
Copy link
Contributor

cserwen commented Apr 28, 2022 •
edited
Loading

@cserwen hi, i have fixed the bug in #139, except (DLedgerMappedFileStoreTest # testAbnormalRecovery ), it can't even run successfully in the old version

I can successfully execute the test in the latest version. Maybe it's failed because of other reasons, not a bug.:-D

@RongtongJin RongtongJin deleted the statemachine branch July 6, 2022 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odbozhou odbozhou odbozhou approved these changes

@yuz10 yuz10 yuz10 approved these changes

@ShannonDing ShannonDing ShannonDing approved these changes

@cserwen cserwen cserwen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RongtongJin @hzh0425 @cserwen @odbozhou @yuz10 @ShannonDing