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

feat(iterator):Auto commit mode for applying log iterator #962

Merged
merged 7 commits into from
Apr 27, 2023
Merged

feat(iterator):Auto commit mode for applying log iterator #962

merged 7 commits into from
Apr 27, 2023

Conversation

1294566108
Copy link
Contributor

Motivation:

Auto commit mode for applying log iterator

Modification:

issue link is here.

Result:

@killme2008
Copy link
Contributor

killme2008 commented Apr 10, 2023

@1294566108 I think we should call commit in Iterator#next(). It means committing the state machine before getting the next element, just like we don't have auto-commit mode but we want to commit on every log:

while(it.hasNext()) {
   byte [] data = it.getData();
   it.commit();
   it.next();   
}

It looks like we don't need the variable lastCommit, commit many times for the same log index has no side effects.

@killme2008
Copy link
Contributor

@1294566108 Please sign the CLA and format the code by mvn compile:compile.

@1294566108
Copy link
Contributor Author

@1294566108 Please sign the CLA and format the code by mvn compile:compile.

Okay, I have recompiled and submitted it

Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

There are a few suggestions. Please sign the CLA http://cla.sofastack.tech/

Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

LGTM

@killme2008
Copy link
Contributor

@1294566108 You have to sign the CLA http://cla.sofastack.tech/

If not, we can't merge this PR. thank you.

@1294566108
Copy link
Contributor Author

@1294566108 You have to sign the CLA http://cla.sofastack.tech/

If not, we can't merge this PR. thank you.

之前似乎有签署过个人的CLA,刚刚又签了一次,请问还需要签署公司CLA吗
image

@killme2008 killme2008 closed this Apr 12, 2023
@killme2008 killme2008 reopened this Apr 12, 2023
@killme2008
Copy link
Contributor

@seeflood Hi, the new contributor's CLA state can't be updated again, please help to check it.

@killme2008 killme2008 merged commit 26977b0 into sofastack:master Apr 27, 2023
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.

3 participants