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

Issue 709: Add Slow Bookkeeper Servers to Placement Policy for read ordering #883

Closed
wants to merge 13 commits into from

Conversation

philipsu522
Copy link

Descriptions of the changes in this PR:

Maintain a list of slow bookkeeper hosts, which are tried only after readonly bookkeeper hosts are tried. Bookkeeper hosts can be categorized as "slow" when a speculative timeout occurs on that bookkeeper host.

Master Issue:: 709

Copy link

@yzang yzang left a comment

Choose a reason for hiding this comment

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

LGTM

}
if (firstRemote != -1) {
int i = 0;
for (;i < remoteNodeInReorderSequence
Copy link

Choose a reason for hiding this comment

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

We can simplify here? for(i=0; i<...; i++)

Copy link
Member

@jiazhai jiazhai left a comment

Choose a reason for hiding this comment

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

+1, lgtm

@sijie
Copy link
Member

sijie commented Dec 19, 2017

thank you @philipsu522 for updating the pull requests.

@sijie
Copy link
Member

sijie commented Dec 19, 2017

waiting for the travis ci to complete the verification.

@eolivelli can you take a look at again? I think now the rebase is correct.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1 looks good, thank you @philipsu522

@philipsu522
Copy link
Author

Thanks a lot everyone, sorry for all the trouble in rebasing and merging

@sijie
Copy link
Member

sijie commented Dec 19, 2017

retest this please.

Copy link
Contributor

@ivankelly ivankelly left a comment

Choose a reason for hiding this comment

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

lgtm. great work philip!

@sijie
Copy link
Member

sijie commented Dec 19, 2017

ci passed on jdk9, some failures on jdk8 but unrelated to this change. merging it now.

@sijie
Copy link
Member

sijie commented Dec 19, 2017

merged this change to master. @philipsu522 great contribution! well done.

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.

None yet

6 participants