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

*: introduce confchange package #383

Merged
merged 3 commits into from Jul 10, 2020
Merged

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Jul 8, 2020

The package is ported from etcd/raft/confchange. restore function is not
used yet. tests will be ported after all functional codes are ported.

There is a difference between raft-rs and etcd that etcd shallow clones
progress set in changer, but raft-rs only use reference and record
changes. It's not easy to shallow clone in rust without lifetime hack
or introducing redundant containers.

The package is ported from etcd/raft/confchange. restore function is not
used yet. tests will be ported after all functional codes are ported.
There is a difference between raft-rs and etcd that etcd shallow clones
progress set in changer, but raft-rs only use reference and record
changes. It's not easy to shallow clone in rust without lifetime hack
or introducing redundant containers.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay BusyJay added the Feature Related to a major feature. label Jul 8, 2020
@BusyJay BusyJay added this to In progress in Joint Consensus via automation Jul 8, 2020
@BusyJay
Copy link
Member Author

BusyJay commented Jul 8, 2020

@Fullstop000 PTAL if you have time. Thanks!

@BusyJay BusyJay moved this from In progress to Needs review in Joint Consensus Jul 8, 2020
}
cfg.voters
.outgoing
.extend(cfg.voters.incoming.iter().cloned());
Copy link
Member

Choose a reason for hiding this comment

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

In etcd, the behavior is clear outgoing and copy incoming to outgoing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The check at L71 ensures that outgoing is empty. Assigning new empty map is necessary in etcd, because it uses nil map which can't be modified. But we use empty map that doesn't have the issue.

/// TODO(tbg) it's silly that this takes a Changer. Unravel this by making sure the
/// Changer only needs a ProgressMap (not a whole Tracker) at which point this can just
/// take LastIndex and MaxInflight directly instead and cook up the results from that
/// alone.
Copy link
Member

Choose a reason for hiding this comment

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

The annotation may should be updated?

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Copy link
Member

@gengliqi gengliqi left a comment

Choose a reason for hiding this comment

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

LGTM

Joint Consensus automation moved this from Needs review to Reviewer approved Jul 10, 2020
@BusyJay BusyJay merged commit dd4ae1e into tikv:master Jul 10, 2020
Joint Consensus automation moved this from Reviewer approved to Done Jul 10, 2020
@BusyJay BusyJay deleted the introduce-confchange branch July 10, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Related to a major feature.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants