Prevent parallel controller pipelines run causing two master replicas

[huizhilu]

Issues

• My PR addresses the following Helix issues and references them in the PR description:

Fixes #1027

Description

• Here are some details about my PR, including screenshots of any UI changes:

There is a case that after controller leader switches, pipelines of both old Helix controller leader and new leader are running in parallel. Different assignment decisions are sent to different participants so there are double masters for a single partition.

The PR addresses this issue by blocking non-leader controller to send messages.

Tests

• The following tests are written for this issue:

• testNoMessageSentOnControllerLosesLeadership

• TestRawZkClient.testAsyncCreateByInvalidSession

• The following is the result of the "mvn test" command on the appropriate module:

[INFO] Tests run: 47, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 61.443 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 47, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:05 min
[INFO] Finished at: 2020-10-04T21:39:20-07:00
[INFO] ------------------------------------------------------------------------


[WARNING] Tests run: 1211, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4,282.946 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[WARNING] Tests run: 1211, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:11 h
[INFO] Finished at: 2020-10-06T04:14:23-07:00
[INFO] ------------------------------------------------------------------------

Commits

• My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation (Optional)

• In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Code Quality

• My diff has been formatted using helix-style.xml
  (helix-style-intellij.xml if IntelliJ IDE is used)

[huizhilu]

Got it. Thanks for pointing that out. I will get a better name for it. Maybe "ZkSessionWrapper". If you have a better name, please let me know.

…

On Fri, Jun 5, 2020 at 12:52 PM Lei Xia ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In helix-core/src/main/java/org/apache/helix/HelixManager.java <#1066 (comment)>: > @@ -425,6 +426,19 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l */ boolean isLeader(); + /** + * Checks if the cluster manager is leader and sets its ZK session in + * ***@***.*** ControllerLeaderSession}. + * + * @param controllerLeaderSession To include ZK session of the cluster manager in return + * + * @return true if this is a controller and a leader of the cluster. Zk session of the cluster + * manager is set in controllerLeaderSession + */ + default boolean isLeader(ControllerLeaderSession controllerLeaderSession) { This could be confusion because there is no "leader" concept for a Helix manager. Leader only applies to a controller instance, but HelixManager is a broader concept here. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1066 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHSRCM4ZMRIF3VAWDW7VQTRVFEHHANCNFSM4NU6ODJA> .

[kaisun2000]

The current pull request has one important assumption if I understand correctly.

Controller assignment to clusters in super cluster change changes when controller has a session expiration and establishing a new session.

The thing is that is there any other event can cause controller assignment to cluster change without controller going through session expiration. For example, new cluster are added? Or any others?
@jiajunwang, @pkuwm?

[huizhilu]

A high level question. The following approach basically encode the session info to the data of ZkClient? Last time in the design discussion I guess people raised the concern that this mean ZkClient need to understand the data format. Normally lower level do not understand higher level data.

Though you can also think of session id in the data as meta data, just like TCP has out of line control data.

Either way is fine to me. Just want to make sure @pkuwm and @lei-xia agree with this approach.

  private String parseExpectedSessionId(Object data) {
    if (!(data instanceof SessionAwareZkWriteData)) {
      return null;
    }
    return ((SessionAwareZkWriteData) data).getExpectedSessionId();

We add a new interface SessionAwareZkWriteData to make it generic: representing data is session aware. In future, we may implement this interface to have a public one like SessionAwareZNRecord . With this new interface, ZkClient doesn’t need to change: treating SessionAwareZkWriteData as generic one.

How about this? @lei-xia @jiajunwang @kaisun2000
Considering the new interface would tie session to zk, it is not generic enough. Eg. if we change the storage to mysql, it doesn’t have session.
How about this, splitting the PR into 2: 1st one (this one) doesn’t have new interface, 2nd one will have the new interface. If we don’t agree on adding new interface, we just discard the 2nd PR, being tracked in #1219.

[kaisun2000]

Overall it looks good to me.

Raise one question about Pause signal added immediately with a lead controller switch case? Can we double check?

If this is indeed a concern. Let us file another ticket to track it.

Also resolve JJ's comment, it seems minor at this point.

[jiajunwang]

LGTM!

[kaisun2000]

@pkuwm, @jiajunwang,

Sync-ed with @pkuwm offline. As I raised, there seems to be another race condition in this code. This is not introduced by this pull request, it exists all the time.

Basically, it is like this:

Say, pause signal is added, the old lead controller see this one and the callback is in zkclient queue. the new lead controller to be also see it, but the generic controller ignored it as it is not leader yet.
then new controller becomes leader due to old controller session expiration.
will new controller in this case not pausing?

Let us sync about shall we fix it here, or in another pull, or what should we do? I will create an issue to track it.

#1442 is tracking this newly raised issue.

[huizhilu]

@pkuwm, @jiajunwang,

Sync-ed with @pkuwm offline. As I raised, there seems to be another race condition in this code. This is not introduced by this pull request, it exists all the time.

Basically, it is like this:

Say, pause signal is added, the old lead controller see this one and the callback is in zkclient queue. the new lead controller to be also see it, but the generic controller ignored it as it is not leader yet.
then new controller becomes leader due to old controller session expiration.
will new controller in this case not pausing?

Let us sync about shall we fix it here, or in another pull, or what should we do? I will create an issue to track it.

#1442 is tracking this newly raised issue.

@kaisun2000 Thanks for creating the issue for tracking it. If it is an issue, we should get it in another PR. Let's discuss.

[huizhilu]

This PR is ready to be merged, approved by @kaisun2000 @jiajunwang

There is a case that after controller leader switches, pipelines of both old Helix controller leader and new leader are running in parallel.
Different assignment decisions are sent to different participants so there are double masters for a single partition.

The commit addresses this issue by blocking non-leader controller to send messages to ZK.