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

Priority-based semi-deterministic leader election. #334

Conversation

zongtanghu
Copy link
Contributor

@zongtanghu zongtanghu commented Nov 6, 2019

Motivation:

The main goal is to prioritize member nodes for next leader election. Raft uses randomized timer so that it is hard to make a deterministic decision. In this library, leader election will be still probabilistic, but more predictable.

Modification:

The main modification of codes is below:
(1)NodeImpl.java;
(2)Node.java;
(3)PeerId.java;
(4)NodeOptions.java;
(5)NodeTest.java;
(6)PeerIdTest.java;
(7)TestUtils.java;
(8)TestCluster.java;
(9)NodeTest;

Result:

Fixes ISSUE #264

If there is no issue then describe the changes introduced by this PR.

@zongtanghu
Copy link
Contributor Author

Hi @fengjiachun @killme2008 ,I have already implemented the initial version of priority-based semi-deterministic leader election.I'm adjusting many test cases in my local enviroment,because this pr refers to many test cases.You can review this pr firstly.And it will be much more efficient if we are reviewing pr's codes and adjusting codes and test cases at the same time.

Copy link
Contributor

@masaimu masaimu left a comment

Choose a reason for hiding this comment

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

提了些问题

@killme2008
Copy link
Contributor

Cool, i will review it ASAP.

@zongtanghu
Copy link
Contributor Author

Cool, i will review it ASAP.

I have already adjust some parts of codes in my local branch and will push again tonight. @killme2008 @masaimu @fengjiachun

@sofastack-bot sofastack-bot bot added size/XL and removed size/L labels Nov 7, 2019
Copy link
Contributor

@masaimu masaimu left a comment

Choose a reason for hiding this comment

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

提了几个问题,可以参考

@zongtanghu
Copy link
Contributor Author

Hi @killme2008 @fengjiachun @masaimu ,I have take the details of priority election into account again.I agree with @fengjiachun .The progress that If candidate_priority < target_priority, it rejects the vote request should be deperated.

@@ -756,7 +948,8 @@ public void testChecksum() throws Exception {
final RaftOptions raftOptions = new RaftOptions();
raftOptions.setEnableLogEntryChecksum(true);
for (final PeerId peer : peers) {
assertTrue(cluster.start(peer.getEndpoint(), false, 300, true, null, raftOptions));
assertTrue(cluster.start(peer.getEndpoint(), false, 300, true, null, raftOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

应该可以不改原有单测, start 增加一个方法,原有方法保持默认 disabled 即可吧?
改了单测我很难判断和老版本的兼容性

@@ -779,7 +972,8 @@ public void testChecksum() throws Exception {
raftOptions = new RaftOptions();
raftOptions.setEnableLogEntryChecksum(true);
}
assertTrue(cluster.start(peer.getEndpoint(), false, 300, true, null, raftOptions));
assertTrue(cluster.start(peer.getEndpoint(), false, 300, true, null, raftOptions,
ElectionPriority.Disabled));
Copy link
Contributor

Choose a reason for hiding this comment

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

同上,下面不再重复

server = new RaftGroupService(this.name, new PeerId(listenAddr, 0), nodeOptions, rpcServer);
} else {
server = new RaftGroupService(this.name, new PeerId(listenAddr, 0, priority), nodeOptions, rpcServer);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

需要 if else ? priority 直接传进去就好

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

*
*/
private void checkAndSetConfiguration() {
this.writeLock.lock();
Copy link
Contributor

Choose a reason for hiding this comment

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

这里我比较建议加个布尔值参数, inLock,如果是 false,才去加这个 writeLock,很多调用的地方可能已经在锁里了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

private int getMaxPriorityOfNodes(final List<PeerId> peerIds) {
Requires.requireNonNull(peerIds, "Null peer list");

int maxPriority = Integer.MIN_VALUE;
Copy link
Contributor

Choose a reason for hiding this comment

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

maxPriority初始值还是用当前 node 的 priority 吧,看起来更合理,虽然结果没有区别。

Copy link
Contributor

Choose a reason for hiding this comment

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

这个原来是用当前 node 的 priority 的,是我给了建议改成 Integer.MIN_VALUE, 主要原因是 Node 还有一种 learner 角色,是不包含在入参 peerIds nodes 中的,所以在这里使用 learner node 的 priority 还是有些奇怪的,虽然结果都没区别

@killme2008
Copy link
Contributor

其他没有太多问题了,可以考虑合并 @fengjiachun

@killme2008
Copy link
Contributor

测试有奇怪错误:

41.92s$ mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V
0.04s$ sh ./tools/check_format.sh
Please commit your change before run this shell, un commit files:
 M jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java
./tools/check_format.sh: 7: exit: Illegal number: -1
The command "sh ./tools/check_format.sh" failed and exited with 2 during .

@zongtanghu
Copy link
Contributor Author

zongtanghu commented Nov 15, 2019 via email

@zongtanghu
Copy link
Contributor Author

zongtanghu commented Nov 15, 2019 via email

@zongtanghu
Copy link
Contributor Author

zongtanghu commented Nov 15, 2019 via email

@fengjiachun
Copy link
Contributor

其他没有太多问题了,可以考虑合并 @fengjiachun

这几天我会做一次 jepsen 验证,通过后合并

@fengjiachun fengjiachun changed the base branch from master to feat/priority_tmp November 19, 2019 14:09
@fengjiachun fengjiachun merged commit dbaa630 into sofastack:feat/priority_tmp Nov 19, 2019
fengjiachun added a commit that referenced this pull request Nov 25, 2019
* Priority-based semi-deterministic leader election. (#334)

* [ISSUE#264]Initial implementation of priority-based semi-deterministic leader election.

* [ISSUE#264]Fix issue and polish codes for priority-based semi-deterministic leader election.

* [ISSUE#264]Fix issue and polish codes for priority-based semi-deterministic leader election.

* [ISSUE#264]Fix issue and polish codes for priority-based semi-deterministic leader election.

* [ISSUE#264]Adding text cases and codes comments for priority-based semi-deterministic leader election.

* [ISSUE#264]optimize codes and method for priority-based semi-deterministic leader election.

* [ISSUE#264]optimize codes and method for priority-based semi-deterministic leader election.

* [ISSUE#264]optimize codes and method for priority-based semi-deterministic leader election.

* [ISSUE#264]optimize codes and method for priority-based semi-deterministic leader election.

* [ISSUE#264]Fix and optimize codes for Priority-based semi-deterministic leader election.

* [ISSUE#264]Fix and optimize codes for Priority-based semi-deterministic leader election.

* [ISSUE#264]adjust codes for priority-based semi-deterministic leader election.

* [ISSUE#264]Fix test codes for Priority-based semi-deterministic leader election.

* minor fix

* minor fix with priority text

* minor fix
@fengjiachun fengjiachun mentioned this pull request Nov 29, 2019
11 tasks
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.

4 participants