Skip to content

Conversation

@XComp
Copy link
Contributor

@XComp XComp commented May 24, 2023

What is the purpose of the change

Makes DefaultLeaderElectionService implement MultipleComponentLeaderElectionDriver.Listener besides the LeaderElectionEventHandler interface.

Brief change log

  • Makes DefaultLeaderElectionService implement MultipleComponentLeaderElectionDriver.Listener
  • Modifies MultipleComponentLeaderElectionDriver.Listener in a way that isLeader requires the session ID to be passed in instead of generating it internally. This aligns more with how LeaderElectionEventHandler was implemented. It also makes more sense because the HA backend should be the owner of the session ID: Theoretically, it could provide such a session ID.

Verifying this change

  • The test code is adapted in a way that it only uses the MultipleComponentLeaderElectionDriver.Listener methods.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: yes
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

@flinkbot
Copy link
Collaborator

flinkbot commented May 24, 2023

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

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

I don't really follow the UUID change. If the multiCLES got by without it so far, what do we need it for now?

the HA backend should be the owner of the session ID: Theoretically, it could provide such a session ID.

Sure, is either ZK/k8s doing that though? Would we gain anything by it?

@XComp
Copy link
Contributor Author

XComp commented Jun 14, 2023

I don't really follow the UUID change. If the multiCLES got by without it so far, what do we need it for now?

the HA backend should be the owner of the session ID: Theoretically, it could provide such a session ID.

Sure, is either ZK/k8s doing that though? Would we gain anything by it?

The initial motivation was to align it with LeaderElectionEventHandler#onGrantLeadership to reduce the effort in migrating the legacy tests to the new interface (it feels like there are more tests for the legacy code; but I didn't count). I found it handy in tests where the test code can define the sessionID as the expected one and we can rely on it without needing to extract it from somewhere. Also from a semantical point of view it made sense (as it is stated in the comment you quoted).

But I can hide the sessionID again to reduce the diff. I might have overestimated the consequences of keeping it the way the LeaderElectionEventHander is doing it

@XComp
Copy link
Contributor Author

XComp commented Jun 15, 2023

New rebase onto FLINK-32177 that fixes the conflicts with master

@XComp
Copy link
Contributor Author

XComp commented Jun 16, 2023

Rebase after adding the checkstyle fix to FLINK-32177 and rebasing it to master

XComp added 4 commits June 16, 2023 17:42
…ultipleComponentLeaderElectionDriver.Listener

Signed-off-by: Matthias Pohl <matthias.pohl@aiven.io>
Signed-off-by: Matthias Pohl <matthias.pohl@aiven.io>
…tener.isLeader again

Signed-off-by: Matthias Pohl <matthias.pohl@aiven.io>
Signed-off-by: Matthias Pohl <matthias.pohl@aiven.io>
@XComp
Copy link
Contributor Author

XComp commented Jun 16, 2023

Rebased to master after FLINK-32177 was merged.

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