You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@flink.apache.org by Matthias Pohl <ma...@aiven.io.INVALID> on 2023/01/05 13:49:50 UTC

[DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Hi everyone,
I brought up FLINK-26522 [1] in the mailing list discussion about
consolidating the HighAvailabilityServices interfaces [2], previously.
There, it was concluded that the community still wants the ability to have
per-component leader election and, therefore, keep the
HighAvailabilityServices interface as is. I went back to work on
FLINK-26522 [1] to figure out how we can simplify the current codebase
keeping the decision in mind.

I wanted to handle FLINK-26522 [1] as a follow-up cleanup task of
FLINK-24038 [3]. But while working on it, I realized that even FLINK-24038
[3] shouldn't have been handled without a FLIP. The per-process leader
election which was introduced in FLINK-24038 [3] changed the ownership of
certain components. This is actually a change that should have been
discussed in the mailing list and deserved a FLIP. To overcome this
shortcoming of FLINK-24038 [3], I decided to prepare FLIP-285 [4] to
provide proper documentation of what happened in FLINK-24038 and what will
be manifested with resolving its follow-up FLINK-26522 [1].

Conceptually, this FLIP proposes moving away from Flink's support for
single-contender LeaderElectionServices and introducing multi-contender
support by disconnecting the HA-backend leader election lifecycle from the
LeaderContender's lifecycle. This allows us to provide LeaderElection per
component (as it was requested in [2]) but also enables us to utilize a
single leader election for multiple components/contenders as well without
the complexity of the code that was introduced by FLINK-24038 [3].

I'm looking forward to your comments.

Matthias

[1] https://issues.apache.org/jira/browse/FLINK-26522
[2] https://lists.apache.org/thread/9oy2ml9s3j1v6r77h31sndhc3gw57cfm
[3] https://issues.apache.org/jira/browse/FLINK-24038
[4]
https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Yang Wang <wa...@apache.org>.

Thanks Matthias for the detailed explanation.

For the HA backend data structure, you are right. Even the different
components are running in a same JVM,
they have completely different connection infos. But it is not urgent to
use a single ZNode to store multiple connection entries for now.

I lean towards not to introduce the *LeaderElection *since it does not take
many benefits.

BTW, if the *LeaderElectionService#register(return LeaderElection)* and
*LeaderElectionService#onGrantLeadership* are guarded by a same lock, then
we could
ensure that the leaderElection in *LeaderContender* is always non-null
when it tries
to confirm the leadership. And then we do not need the
*LeaderContender#initializeLeaderElection*.
Right?


Best,
Yang

Matthias Pohl <ma...@aiven.io.invalid> 于2023年1月17日周二 20:31写道：

> Thanks Yang for getting back to me.
>
> I checked the connection information that's stored in the HA backend once
> more. My previous proposal is based on a wrong assumption: The address we
> store is the RPC endpoint's address. That address should be unique per
> component which means that we shouldn't change how the HA backend stores
> the connection information. We don't store redundant information here. As a
> consequence, I had to reiterate over my proposal in FLIP-285. We're now
> relying on contender IDs that are going to be used internally for storing
> the connection information in the HA backend (analogously to how it's done
> in the MultipleComponentLeaderElectionService implementation right now).
>
> Additionally, I came to the conclusion that it's actually not really
> necessary to add the LeaderElection interface. Working with the contender
> IDs to identify the LeaderContender in the LeaderElectionService might be
> good enough. But I still kept the LeaderElection interface as an (optional)
> extension of FLIP-285 as it might improve testability a bit. I added some
> diagrams and descriptions to the FLIP hoping that this helps answer your
> questions, Yang.
>
> Best,
> Matthias
>
> On Mon, Jan 16, 2023 at 8:41 AM Yang Wang <da...@gmail.com> wrote:
>
> > Thanks Matthias for updating the FLIP.
> >
> > Given that we do not touch the dedicated ConfigMap for the checkpoint,
> this
> > FLIP will not affect the LAST_STATE recovery for the Flink Kubernetes
> > Operator.
> >
> > # LeaderContender#initiateLeaderElection
> > Could we simply execute *leaderElectionService.register(contender) *just
> in
> > the same place with current *leaderElectionService.start(contender)*, not
> > within the constructor?
> > Then we could ensure that the *onGrantLeadership* does not happen before
> > the contender is ready.
> >
> > # LeaderElection#confirmLeadership
> > Could you please share more about how *LeaderElection#confirmLeadership
> > *works?
> > It will directly call the *LeaderElectionDriver#writeLeaderInformation*,
> or
> > *LeaderElectionService*
> > will be the adapter.
> >
> > # LeaderRetrievalService
> > This might be out the scope of this FLIP :)
> > Do we need to introduce a corresponding
> > *MultipleComponentLeaderRetrievalService*, especially when we are using
> > only one ZNode/ConfigMap for storing all the leader information?
> > For Kubernetes HA, we have already use a shared ConfigMap watcher for all
> > the leader retrieval services.
> > However, for ZK HA, each leader retrieval service still has a dedicated
> > *TreeCache*.
> >
> >
> > Best,
> > Yang
> >
> >
> >
> > Matthias Pohl <ma...@aiven.io.invalid> 于2023年1月12日周四 22:07写道：
> >
> > > Thanks Yang Wang for sharing your view on this. Please find my
> responses
> > > below.
> > >
> > > # HA data format in the HA backend(e.g. ZK, K8s ConfigMap)
> > > > We have already changed the HA data format after introducing the
> > multiple
> > > > component leader election in FLINK-24038. For K8s HA,
> > > > the num of ConfigMaps reduced from 4 to 2. Since we only have one
> > leader
> > > > elector, the K8s APIServer load should also be reduced.
> > > > Why do we still need to change the format again? This just prevents
> the
> > > > LAST_STATE upgrade mode in Flink-Kubernetes-Operator
> > > > when the Flink version changed, even though it is a simple job and
> > state
> > > is
> > > > compatible.
> > > >
> > >
> > > The intention of this remark is that we could reduce the number of
> > > redundant records (the
> > ZooKeeperMultipleComponentLeaderElectionHaServices'
> > > JavaDoc [1] visualizes the redundancy quite well since each of these
> > > connection_info records would contain the very same information). We're
> > > saving the same connection_info for each of the componentIds (e.g.
> > > resource_manager, dispatcher, ...) right now. My rationale was that we
> > only
> > > need to save the connection info once per LeaderElectionDriver, i.e.
> > > LeaderElectionService. It's an implementation detail of the
> > > LeaderElectionService implementation to know what components it owns.
> > > Therefore, I suggested that we would have a unique ID per
> > > LeaderElectionService instance with a single connection_info that is
> used
> > > by all the components that are registered to that service. If we decide
> > to
> > > have a separate LeaderElectionService for a specific component (e.g.
> the
> > > resource manager) in the future, this would end up having a separate
> > > ConfigMap in k8s or separate ZNode in ZooKeeper.
> > >
> > > I added these details to the FLIP [2]. That part, indeed, was quite
> > poorly
> > > described there initally.
> > >
> > > I don't understand how the leader election affects the LAST_STATE
> changes
> > > in the Kubernetes Operator, though. We use a separate ConfigMap for the
> > > checkpoint data [3]. Can you elaborate a little bit more on your
> concern?
> > >
> > > [1]
> > >
> > >
> >
> https://github.com/apache/flink/blob/8ddfd590ebba7fc727e79db41b82d3d40a02b56a/flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/zookeeper/ZooKeeperMultipleComponentLeaderElectionHaServices.java#L47-L61
> > > [2]
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box#ha-backend-data-schema
> > > [3]
> > >
> > >
> >
> https://github.com/apache/flink/blob/2770acee1bc4a82a2f4223d4a4cd6073181dc840/flink-kubernetes/src/main/java/org/apache/flink/kubernetes/highavailability/KubernetesMultipleComponentLeaderElectionHaServices.java#L163
> > >
> > >
> > > > # LeaderContender#initiateLeaderElection
> > > > I do not get your point why we need the *initiateLeaderElection* in
> > > > *LeaderContender*. AFAICS, the callback *onGrant/RevokeLeadership*
> > > > could be executed as soon as the registration.
> > > >
> > >
> > > That's the change I'm not really happy about. I'm still not sure
> whether
> > I
> > > found the best solution here. The problem is the way the components are
> > > initialized. The initial plan was to call the
> > > LeaderElectionService.register(LeaderContender) from within the
> > > LeaderContender constructor which would return the LeaderElection
> > instance
> > > that would be used as the adapter for the contender to confirm
> > leadership.
> > > Therefore, the LeaderContender has to own the LeaderElection instance
> to
> > be
> > > able to participate in the leader election handshake (i.e. grant
> > leadership
> > > -> confirm leadership). With that setup, we wouldn't be able to grant
> > > leadership during the LeaderElectionService.register(LeaderContender)
> > call
> > > because that would require for the LeaderContender to confirm the
> > > leadership. That is not possible because the LeaderElection wasn't
> > created
> > > and set within the LeaderContender, yet. Therefore, we have to have
> some
> > > means to initialize the LeaderElection before triggering granting the
> > > leadership from within the LeaderElectionService. ...and that's what
> this
> > > method is for.
> > >
> > > Best,
> > > Matthias
> > >
> >
>

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Matthias Pohl <ma...@aiven.io.INVALID>.

Thanks Yang for getting back to me.

I checked the connection information that's stored in the HA backend once
more. My previous proposal is based on a wrong assumption: The address we
store is the RPC endpoint's address. That address should be unique per
component which means that we shouldn't change how the HA backend stores
the connection information. We don't store redundant information here. As a
consequence, I had to reiterate over my proposal in FLIP-285. We're now
relying on contender IDs that are going to be used internally for storing
the connection information in the HA backend (analogously to how it's done
in the MultipleComponentLeaderElectionService implementation right now).

Additionally, I came to the conclusion that it's actually not really
necessary to add the LeaderElection interface. Working with the contender
IDs to identify the LeaderContender in the LeaderElectionService might be
good enough. But I still kept the LeaderElection interface as an (optional)
extension of FLIP-285 as it might improve testability a bit. I added some
diagrams and descriptions to the FLIP hoping that this helps answer your
questions, Yang.

Best,
Matthias

On Mon, Jan 16, 2023 at 8:41 AM Yang Wang <da...@gmail.com> wrote:

> Thanks Matthias for updating the FLIP.
>
> Given that we do not touch the dedicated ConfigMap for the checkpoint, this
> FLIP will not affect the LAST_STATE recovery for the Flink Kubernetes
> Operator.
>
> # LeaderContender#initiateLeaderElection
> Could we simply execute *leaderElectionService.register(contender) *just in
> the same place with current *leaderElectionService.start(contender)*, not
> within the constructor?
> Then we could ensure that the *onGrantLeadership* does not happen before
> the contender is ready.
>
> # LeaderElection#confirmLeadership
> Could you please share more about how *LeaderElection#confirmLeadership
> *works?
> It will directly call the *LeaderElectionDriver#writeLeaderInformation*, or
> *LeaderElectionService*
> will be the adapter.
>
> # LeaderRetrievalService
> This might be out the scope of this FLIP :)
> Do we need to introduce a corresponding
> *MultipleComponentLeaderRetrievalService*, especially when we are using
> only one ZNode/ConfigMap for storing all the leader information?
> For Kubernetes HA, we have already use a shared ConfigMap watcher for all
> the leader retrieval services.
> However, for ZK HA, each leader retrieval service still has a dedicated
> *TreeCache*.
>
>
> Best,
> Yang
>
>
>
> Matthias Pohl <ma...@aiven.io.invalid> 于2023年1月12日周四 22:07写道：
>
> > Thanks Yang Wang for sharing your view on this. Please find my responses
> > below.
> >
> > # HA data format in the HA backend(e.g. ZK, K8s ConfigMap)
> > > We have already changed the HA data format after introducing the
> multiple
> > > component leader election in FLINK-24038. For K8s HA,
> > > the num of ConfigMaps reduced from 4 to 2. Since we only have one
> leader
> > > elector, the K8s APIServer load should also be reduced.
> > > Why do we still need to change the format again? This just prevents the
> > > LAST_STATE upgrade mode in Flink-Kubernetes-Operator
> > > when the Flink version changed, even though it is a simple job and
> state
> > is
> > > compatible.
> > >
> >
> > The intention of this remark is that we could reduce the number of
> > redundant records (the
> ZooKeeperMultipleComponentLeaderElectionHaServices'
> > JavaDoc [1] visualizes the redundancy quite well since each of these
> > connection_info records would contain the very same information). We're
> > saving the same connection_info for each of the componentIds (e.g.
> > resource_manager, dispatcher, ...) right now. My rationale was that we
> only
> > need to save the connection info once per LeaderElectionDriver, i.e.
> > LeaderElectionService. It's an implementation detail of the
> > LeaderElectionService implementation to know what components it owns.
> > Therefore, I suggested that we would have a unique ID per
> > LeaderElectionService instance with a single connection_info that is used
> > by all the components that are registered to that service. If we decide
> to
> > have a separate LeaderElectionService for a specific component (e.g. the
> > resource manager) in the future, this would end up having a separate
> > ConfigMap in k8s or separate ZNode in ZooKeeper.
> >
> > I added these details to the FLIP [2]. That part, indeed, was quite
> poorly
> > described there initally.
> >
> > I don't understand how the leader election affects the LAST_STATE changes
> > in the Kubernetes Operator, though. We use a separate ConfigMap for the
> > checkpoint data [3]. Can you elaborate a little bit more on your concern?
> >
> > [1]
> >
> >
> https://github.com/apache/flink/blob/8ddfd590ebba7fc727e79db41b82d3d40a02b56a/flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/zookeeper/ZooKeeperMultipleComponentLeaderElectionHaServices.java#L47-L61
> > [2]
> >
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box#ha-backend-data-schema
> > [3]
> >
> >
> https://github.com/apache/flink/blob/2770acee1bc4a82a2f4223d4a4cd6073181dc840/flink-kubernetes/src/main/java/org/apache/flink/kubernetes/highavailability/KubernetesMultipleComponentLeaderElectionHaServices.java#L163
> >
> >
> > > # LeaderContender#initiateLeaderElection
> > > I do not get your point why we need the *initiateLeaderElection* in
> > > *LeaderContender*. AFAICS, the callback *onGrant/RevokeLeadership*
> > > could be executed as soon as the registration.
> > >
> >
> > That's the change I'm not really happy about. I'm still not sure whether
> I
> > found the best solution here. The problem is the way the components are
> > initialized. The initial plan was to call the
> > LeaderElectionService.register(LeaderContender) from within the
> > LeaderContender constructor which would return the LeaderElection
> instance
> > that would be used as the adapter for the contender to confirm
> leadership.
> > Therefore, the LeaderContender has to own the LeaderElection instance to
> be
> > able to participate in the leader election handshake (i.e. grant
> leadership
> > -> confirm leadership). With that setup, we wouldn't be able to grant
> > leadership during the LeaderElectionService.register(LeaderContender)
> call
> > because that would require for the LeaderContender to confirm the
> > leadership. That is not possible because the LeaderElection wasn't
> created
> > and set within the LeaderContender, yet. Therefore, we have to have some
> > means to initialize the LeaderElection before triggering granting the
> > leadership from within the LeaderElectionService. ...and that's what this
> > method is for.
> >
> > Best,
> > Matthias
> >
>

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Yang Wang <da...@gmail.com>.

Thanks Matthias for updating the FLIP.

Given that we do not touch the dedicated ConfigMap for the checkpoint, this
FLIP will not affect the LAST_STATE recovery for the Flink Kubernetes
Operator.

# LeaderContender#initiateLeaderElection
Could we simply execute *leaderElectionService.register(contender) *just in
the same place with current *leaderElectionService.start(contender)*, not
within the constructor?
Then we could ensure that the *onGrantLeadership* does not happen before
the contender is ready.

# LeaderElection#confirmLeadership
Could you please share more about how *LeaderElection#confirmLeadership *works?
It will directly call the *LeaderElectionDriver#writeLeaderInformation*, or
*LeaderElectionService*
will be the adapter.

# LeaderRetrievalService
This might be out the scope of this FLIP :)
Do we need to introduce a corresponding
*MultipleComponentLeaderRetrievalService*, especially when we are using
only one ZNode/ConfigMap for storing all the leader information?
For Kubernetes HA, we have already use a shared ConfigMap watcher for all
the leader retrieval services.
However, for ZK HA, each leader retrieval service still has a dedicated
*TreeCache*.


Best,
Yang



Matthias Pohl <ma...@aiven.io.invalid> 于2023年1月12日周四 22:07写道：

> Thanks Yang Wang for sharing your view on this. Please find my responses
> below.
>
> # HA data format in the HA backend(e.g. ZK, K8s ConfigMap)
> > We have already changed the HA data format after introducing the multiple
> > component leader election in FLINK-24038. For K8s HA,
> > the num of ConfigMaps reduced from 4 to 2. Since we only have one leader
> > elector, the K8s APIServer load should also be reduced.
> > Why do we still need to change the format again? This just prevents the
> > LAST_STATE upgrade mode in Flink-Kubernetes-Operator
> > when the Flink version changed, even though it is a simple job and state
> is
> > compatible.
> >
>
> The intention of this remark is that we could reduce the number of
> redundant records (the ZooKeeperMultipleComponentLeaderElectionHaServices'
> JavaDoc [1] visualizes the redundancy quite well since each of these
> connection_info records would contain the very same information). We're
> saving the same connection_info for each of the componentIds (e.g.
> resource_manager, dispatcher, ...) right now. My rationale was that we only
> need to save the connection info once per LeaderElectionDriver, i.e.
> LeaderElectionService. It's an implementation detail of the
> LeaderElectionService implementation to know what components it owns.
> Therefore, I suggested that we would have a unique ID per
> LeaderElectionService instance with a single connection_info that is used
> by all the components that are registered to that service. If we decide to
> have a separate LeaderElectionService for a specific component (e.g. the
> resource manager) in the future, this would end up having a separate
> ConfigMap in k8s or separate ZNode in ZooKeeper.
>
> I added these details to the FLIP [2]. That part, indeed, was quite poorly
> described there initally.
>
> I don't understand how the leader election affects the LAST_STATE changes
> in the Kubernetes Operator, though. We use a separate ConfigMap for the
> checkpoint data [3]. Can you elaborate a little bit more on your concern?
>
> [1]
>
> https://github.com/apache/flink/blob/8ddfd590ebba7fc727e79db41b82d3d40a02b56a/flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/zookeeper/ZooKeeperMultipleComponentLeaderElectionHaServices.java#L47-L61
> [2]
>
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box#ha-backend-data-schema
> [3]
>
> https://github.com/apache/flink/blob/2770acee1bc4a82a2f4223d4a4cd6073181dc840/flink-kubernetes/src/main/java/org/apache/flink/kubernetes/highavailability/KubernetesMultipleComponentLeaderElectionHaServices.java#L163
>
>
> > # LeaderContender#initiateLeaderElection
> > I do not get your point why we need the *initiateLeaderElection* in
> > *LeaderContender*. AFAICS, the callback *onGrant/RevokeLeadership*
> > could be executed as soon as the registration.
> >
>
> That's the change I'm not really happy about. I'm still not sure whether I
> found the best solution here. The problem is the way the components are
> initialized. The initial plan was to call the
> LeaderElectionService.register(LeaderContender) from within the
> LeaderContender constructor which would return the LeaderElection instance
> that would be used as the adapter for the contender to confirm leadership.
> Therefore, the LeaderContender has to own the LeaderElection instance to be
> able to participate in the leader election handshake (i.e. grant leadership
> -> confirm leadership). With that setup, we wouldn't be able to grant
> leadership during the LeaderElectionService.register(LeaderContender) call
> because that would require for the LeaderContender to confirm the
> leadership. That is not possible because the LeaderElection wasn't created
> and set within the LeaderContender, yet. Therefore, we have to have some
> means to initialize the LeaderElection before triggering granting the
> leadership from within the LeaderElectionService. ...and that's what this
> method is for.
>
> Best,
> Matthias
>

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Matthias Pohl <ma...@aiven.io.INVALID>.

Thanks Yang Wang for sharing your view on this. Please find my responses
below.

# HA data format in the HA backend(e.g. ZK, K8s ConfigMap)
> We have already changed the HA data format after introducing the multiple
> component leader election in FLINK-24038. For K8s HA,
> the num of ConfigMaps reduced from 4 to 2. Since we only have one leader
> elector, the K8s APIServer load should also be reduced.
> Why do we still need to change the format again? This just prevents the
> LAST_STATE upgrade mode in Flink-Kubernetes-Operator
> when the Flink version changed, even though it is a simple job and state is
> compatible.
>

The intention of this remark is that we could reduce the number of
redundant records (the ZooKeeperMultipleComponentLeaderElectionHaServices'
JavaDoc [1] visualizes the redundancy quite well since each of these
connection_info records would contain the very same information). We're
saving the same connection_info for each of the componentIds (e.g.
resource_manager, dispatcher, ...) right now. My rationale was that we only
need to save the connection info once per LeaderElectionDriver, i.e.
LeaderElectionService. It's an implementation detail of the
LeaderElectionService implementation to know what components it owns.
Therefore, I suggested that we would have a unique ID per
LeaderElectionService instance with a single connection_info that is used
by all the components that are registered to that service. If we decide to
have a separate LeaderElectionService for a specific component (e.g. the
resource manager) in the future, this would end up having a separate
ConfigMap in k8s or separate ZNode in ZooKeeper.

I added these details to the FLIP [2]. That part, indeed, was quite poorly
described there initally.

I don't understand how the leader election affects the LAST_STATE changes
in the Kubernetes Operator, though. We use a separate ConfigMap for the
checkpoint data [3]. Can you elaborate a little bit more on your concern?

[1]
https://github.com/apache/flink/blob/8ddfd590ebba7fc727e79db41b82d3d40a02b56a/flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/zookeeper/ZooKeeperMultipleComponentLeaderElectionHaServices.java#L47-L61
[2]
https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box#ha-backend-data-schema
[3]
https://github.com/apache/flink/blob/2770acee1bc4a82a2f4223d4a4cd6073181dc840/flink-kubernetes/src/main/java/org/apache/flink/kubernetes/highavailability/KubernetesMultipleComponentLeaderElectionHaServices.java#L163


> # LeaderContender#initiateLeaderElection
> I do not get your point why we need the *initiateLeaderElection* in
> *LeaderContender*. AFAICS, the callback *onGrant/RevokeLeadership*
> could be executed as soon as the registration.
>

That's the change I'm not really happy about. I'm still not sure whether I
found the best solution here. The problem is the way the components are
initialized. The initial plan was to call the
LeaderElectionService.register(LeaderContender) from within the
LeaderContender constructor which would return the LeaderElection instance
that would be used as the adapter for the contender to confirm leadership.
Therefore, the LeaderContender has to own the LeaderElection instance to be
able to participate in the leader election handshake (i.e. grant leadership
-> confirm leadership). With that setup, we wouldn't be able to grant
leadership during the LeaderElectionService.register(LeaderContender) call
because that would require for the LeaderContender to confirm the
leadership. That is not possible because the LeaderElection wasn't created
and set within the LeaderContender, yet. Therefore, we have to have some
means to initialize the LeaderElection before triggering granting the
leadership from within the LeaderElectionService. ...and that's what this
method is for.

Best,
Matthias

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Yang Wang <da...@gmail.com>.

Thanks Matthias for preparing this thorough FLIP, which has taken us
reviewing the multiple component leader election.

I am totally in favor of doing the code clean-up. The current
implementation does not have very good readability due to legacy
compatibility.
And I just have a few comments.

# HA data format in the HA backend(e.g. ZK, K8s ConfigMap)
We have already changed the HA data format after introducing the multiple
component leader election in FLINK-24038. For K8s HA,
the num of ConfigMaps reduced from 4 to 2. Since we only have one leader
elector, the K8s APIServer load should also be reduced.
Why do we still need to change the format again? This just prevents the
LAST_STATE upgrade mode in Flink-Kubernetes-Operator
when the Flink version changed, even though it is a simple job and state is
compatible.

# LeaderContender#initiateLeaderElection
I do not get your point why we need the *initiateLeaderElection* in
*LeaderContender*. AFAICS, the callback *onGrant/RevokeLeadership*
could be executed as soon as the registration.

# When to establish the HA backend connection
+1 for establish the connection beforehand


Best,
Yang

Matthias Pohl <ma...@aiven.io.invalid> 于2023年1月5日周四 21:51写道：

> Hi everyone,
> I brought up FLINK-26522 [1] in the mailing list discussion about
> consolidating the HighAvailabilityServices interfaces [2], previously.
> There, it was concluded that the community still wants the ability to have
> per-component leader election and, therefore, keep the
> HighAvailabilityServices interface as is. I went back to work on
> FLINK-26522 [1] to figure out how we can simplify the current codebase
> keeping the decision in mind.
>
> I wanted to handle FLINK-26522 [1] as a follow-up cleanup task of
> FLINK-24038 [3]. But while working on it, I realized that even FLINK-24038
> [3] shouldn't have been handled without a FLIP. The per-process leader
> election which was introduced in FLINK-24038 [3] changed the ownership of
> certain components. This is actually a change that should have been
> discussed in the mailing list and deserved a FLIP. To overcome this
> shortcoming of FLINK-24038 [3], I decided to prepare FLIP-285 [4] to
> provide proper documentation of what happened in FLINK-24038 and what will
> be manifested with resolving its follow-up FLINK-26522 [1].
>
> Conceptually, this FLIP proposes moving away from Flink's support for
> single-contender LeaderElectionServices and introducing multi-contender
> support by disconnecting the HA-backend leader election lifecycle from the
> LeaderContender's lifecycle. This allows us to provide LeaderElection per
> component (as it was requested in [2]) but also enables us to utilize a
> single leader election for multiple components/contenders as well without
> the complexity of the code that was introduced by FLINK-24038 [3].
>
> I'm looking forward to your comments.
>
> Matthias
>
> [1] https://issues.apache.org/jira/browse/FLINK-26522
> [2] https://lists.apache.org/thread/9oy2ml9s3j1v6r77h31sndhc3gw57cfm
> [3] https://issues.apache.org/jira/browse/FLINK-24038
> [4]
>
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
>

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Matthias Pohl <ma...@aiven.io.INVALID>.

Thanks Yang and Chesnay for your feedback. I'm gonna go ahead and start a
voting thread as this discussion thread is already open for some time.

Best,
Matthias

On Wed, Jan 25, 2023 at 4:08 AM Yang Wang <wa...@apache.org> wrote:

> Having the *start()* in *LeaderContender* interface and bringing back the
> *LeaderElection* with some new methods make sense to me.
>
> I have no more concerns now.
>
>
> >    - *LeaderContender*: The LeaderContender is integrated as usual except
> >    that it accesses the LeaderElection instead of the
> LeaderElectionService.
> >    It's going to call startLeaderElection(LeaderContender) where,
> previously,
> >    LeaderElectionService.start(LeaderContender) was called.
> >
> > nit: we call the *LeaderElection#startLeaderElection()*, not the
> *LeaderElection#startLeaderElection(LeaderContender)*. Because we have
> already set the leaderContender in the
> *LeaderElection#register(LeaderContender)*.
>
>
> Best,
> Yang
>
>
> Chesnay Schepler <ch...@apache.org> 于2023年1月23日周一 23:16写道：
>
> > Thanks for updating the design. From my side this looks good.
> >
> > On 18/01/2023 17:59, Matthias Pohl wrote:
> > > After another round of discussion, I came up with a (hopefully) final
> > > proposal. The previously discussed approach was still not optimal
> because
> > > the contender ID lived in the LeaderContender even though it is
> actually
> > > LeaderElectionService-internal knowledge. Fixing that helped fix the
> > > overall architecture. Additionally, it brought back the LeaderElection
> > > interface with slightly different methods.
> > >
> > > I updated the "Code Cleanup: Merge
> MultipleComponentLeaderElectionService
> > > into LeaderElectionService" section and moved the old proposal into the
> > > section for rejected alternatives. Feel free to have another look at
> the
> > > updated version [1].
> > >
> > > Matthias
> > >
> > > [1]
> > >
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
> > >
> > > On Wed, Jan 18, 2023 at 1:40 PM Matthias Pohl <ma...@aiven.io>
> > > wrote:
> > >
> > >> Thanks for participating in the discussion, Yang & Chesnay.
> > LeaderElection
> > >> interface extension gave me a headache as well. I added it initially
> > >> because I thought it would be of more value. But essentially, it
> doesn't
> > >> help but make the code harder to understand (as your questions
> > rightfully
> > >> point out). I agree that the FLIP is good enough without this
> > extension. I
> > >> moved it into the Rejected Alternatives section of the FLIP and would
> > >> propose going ahead without it.
> > >>
> > >> I will answer your questions about the LeaderElection extension,
> anyway:
> > >>
> > >> BTW, if the *LeaderElectionService#register(return LeaderElection)*
> and
> > >>> *LeaderElectionService#onGrantLeadership* are guarded by a same lock,
> > then
> > >>> we could ensure that the leaderElection in *LeaderContender* is
> always
> > >>> non-null when it tries to confirm the leadership. And then we do not
> > need
> > >>> the
> > >>> *LeaderContender#initializeLeaderElection*. Right?
> > >>
> > >> No, we still would need LeaderContender#initializeLeaderElection
> because
> > >> the LeaderElectionService needs to be capable of setting the
> > LeaderElection
> > >> within the LeaderContender before triggering the process for granting
> > the
> > >> leadership. This all needs to happen within the
> > >> LeaderElectionService#register(LeaderContender). It's indepent of the
> > lock.
> > >>
> > >> With the extension, how does the leader contender get access to the
> > >>> LeaderElection? I would've assumed that LEService returns a
> > LeaderElection
> > >>> when register is called, but according to the diagram this method
> > doesn't
> > >>> return anything. Is that what initiateLeaderElection is doing?
> > >>
> > >> Correct. My initial plan was to make
> > >> LeaderElectionService#register(LeaderContender) return the
> > LeaderElection
> > >> instance. That method could have been called within the
> LeaderContender.
> > >> But this approach has the flaw that LeaderContender would be in charge
> > >> within this control flow where, actually, we would want
> > >> LeaderElectionService to be still in charge to trigger the process for
> > >> granting the leadership. This required the
> > >> LeaderContender.initializeLeaderElection(LeaderElection) method to be
> > added
> > >> to enable the LeaderElectionService to do the initialization. I added
> a
> > >> comment to the corresponding class diagram to make this clearer.
> > >>
> > >> The DefaultLeaderElection will rely on package-private methods of the
> > >>> DLEService to handle confirm/hasLeadership calls?
> > >>
> > >> Correct. I added the missing package-private methods to the class
> > diagram
> > >> in the FLIP to clear things up.
> > >>
> > >> On Wed, Jan 18, 2023 at 11:47 AM Chesnay Schepler <chesnay@apache.org
> >
> > >> wrote:
> > >>
> > >>> There are a lot of good things in this, and until the Extension bit
> I'm
> > >>> fully on board.
> > >>>
> > >>> With the extension, how does the leader contender get access to the
> > >>> LeaderElection? I would've assumed that LEService returns a
> > >>> LeaderElection when register is called, but according to the diagram
> > >>> this method doesn't return anything. Is that what
> > initiateLeaderElection
> > >>> is doing?
> > >>>
> > >>> The DefaultLeaderElection will rely on package-private methods of the
> > >>> DLEService to handle confirm/hasLeadership calls?
> > >>>
> > >>> I don't fully understand why LContender#initializeLeaderElection is
> > >>> required.
> > >>>
> > >>> On 05/01/2023 14:49, Matthias Pohl wrote:
> > >>>> Hi everyone,
> > >>>> I brought up FLINK-26522 [1] in the mailing list discussion about
> > >>>> consolidating the HighAvailabilityServices interfaces [2],
> previously.
> > >>>> There, it was concluded that the community still wants the ability
> to
> > >>> have
> > >>>> per-component leader election and, therefore, keep the
> > >>>> HighAvailabilityServices interface as is. I went back to work on
> > >>>> FLINK-26522 [1] to figure out how we can simplify the current
> codebase
> > >>>> keeping the decision in mind.
> > >>>>
> > >>>> I wanted to handle FLINK-26522 [1] as a follow-up cleanup task of
> > >>>> FLINK-24038 [3]. But while working on it, I realized that even
> > >>> FLINK-24038
> > >>>> [3] shouldn't have been handled without a FLIP. The per-process
> leader
> > >>>> election which was introduced in FLINK-24038 [3] changed the
> ownership
> > >>> of
> > >>>> certain components. This is actually a change that should have been
> > >>>> discussed in the mailing list and deserved a FLIP. To overcome this
> > >>>> shortcoming of FLINK-24038 [3], I decided to prepare FLIP-285 [4] to
> > >>>> provide proper documentation of what happened in FLINK-24038 and
> what
> > >>> will
> > >>>> be manifested with resolving its follow-up FLINK-26522 [1].
> > >>>>
> > >>>> Conceptually, this FLIP proposes moving away from Flink's support
> for
> > >>>> single-contender LeaderElectionServices and introducing
> > multi-contender
> > >>>> support by disconnecting the HA-backend leader election lifecycle
> from
> > >>> the
> > >>>> LeaderContender's lifecycle. This allows us to provide
> LeaderElection
> > >>> per
> > >>>> component (as it was requested in [2]) but also enables us to
> utilize
> > a
> > >>>> single leader election for multiple components/contenders as well
> > >>> without
> > >>>> the complexity of the code that was introduced by FLINK-24038 [3].
> > >>>>
> > >>>> I'm looking forward to your comments.
> > >>>>
> > >>>> Matthias
> > >>>>
> > >>>> [1] https://issues.apache.org/jira/browse/FLINK-26522
> > >>>> [2]
> https://lists.apache.org/thread/9oy2ml9s3j1v6r77h31sndhc3gw57cfm
> > >>>> [3] https://issues.apache.org/jira/browse/FLINK-24038
> > >>>> [4]
> > >>>>
> > >>>
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
> > >>>
> >
> >
>

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Yang Wang <wa...@apache.org>.

Having the *start()* in *LeaderContender* interface and bringing back the
*LeaderElection* with some new methods make sense to me.

I have no more concerns now.


>    - *LeaderContender*: The LeaderContender is integrated as usual except
>    that it accesses the LeaderElection instead of the LeaderElectionService.
>    It's going to call startLeaderElection(LeaderContender) where, previously,
>    LeaderElectionService.start(LeaderContender) was called.
>
> nit: we call the *LeaderElection#startLeaderElection()*, not the
*LeaderElection#startLeaderElection(LeaderContender)*. Because we have
already set the leaderContender in the
*LeaderElection#register(LeaderContender)*.


Best,
Yang


Chesnay Schepler <ch...@apache.org> 于2023年1月23日周一 23:16写道：

> Thanks for updating the design. From my side this looks good.
>
> On 18/01/2023 17:59, Matthias Pohl wrote:
> > After another round of discussion, I came up with a (hopefully) final
> > proposal. The previously discussed approach was still not optimal because
> > the contender ID lived in the LeaderContender even though it is actually
> > LeaderElectionService-internal knowledge. Fixing that helped fix the
> > overall architecture. Additionally, it brought back the LeaderElection
> > interface with slightly different methods.
> >
> > I updated the "Code Cleanup: Merge MultipleComponentLeaderElectionService
> > into LeaderElectionService" section and moved the old proposal into the
> > section for rejected alternatives. Feel free to have another look at the
> > updated version [1].
> >
> > Matthias
> >
> > [1]
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
> >
> > On Wed, Jan 18, 2023 at 1:40 PM Matthias Pohl <ma...@aiven.io>
> > wrote:
> >
> >> Thanks for participating in the discussion, Yang & Chesnay.
> LeaderElection
> >> interface extension gave me a headache as well. I added it initially
> >> because I thought it would be of more value. But essentially, it doesn't
> >> help but make the code harder to understand (as your questions
> rightfully
> >> point out). I agree that the FLIP is good enough without this
> extension. I
> >> moved it into the Rejected Alternatives section of the FLIP and would
> >> propose going ahead without it.
> >>
> >> I will answer your questions about the LeaderElection extension, anyway:
> >>
> >> BTW, if the *LeaderElectionService#register(return LeaderElection)* and
> >>> *LeaderElectionService#onGrantLeadership* are guarded by a same lock,
> then
> >>> we could ensure that the leaderElection in *LeaderContender* is always
> >>> non-null when it tries to confirm the leadership. And then we do not
> need
> >>> the
> >>> *LeaderContender#initializeLeaderElection*. Right?
> >>
> >> No, we still would need LeaderContender#initializeLeaderElection because
> >> the LeaderElectionService needs to be capable of setting the
> LeaderElection
> >> within the LeaderContender before triggering the process for granting
> the
> >> leadership. This all needs to happen within the
> >> LeaderElectionService#register(LeaderContender). It's indepent of the
> lock.
> >>
> >> With the extension, how does the leader contender get access to the
> >>> LeaderElection? I would've assumed that LEService returns a
> LeaderElection
> >>> when register is called, but according to the diagram this method
> doesn't
> >>> return anything. Is that what initiateLeaderElection is doing?
> >>
> >> Correct. My initial plan was to make
> >> LeaderElectionService#register(LeaderContender) return the
> LeaderElection
> >> instance. That method could have been called within the LeaderContender.
> >> But this approach has the flaw that LeaderContender would be in charge
> >> within this control flow where, actually, we would want
> >> LeaderElectionService to be still in charge to trigger the process for
> >> granting the leadership. This required the
> >> LeaderContender.initializeLeaderElection(LeaderElection) method to be
> added
> >> to enable the LeaderElectionService to do the initialization. I added a
> >> comment to the corresponding class diagram to make this clearer.
> >>
> >> The DefaultLeaderElection will rely on package-private methods of the
> >>> DLEService to handle confirm/hasLeadership calls?
> >>
> >> Correct. I added the missing package-private methods to the class
> diagram
> >> in the FLIP to clear things up.
> >>
> >> On Wed, Jan 18, 2023 at 11:47 AM Chesnay Schepler <ch...@apache.org>
> >> wrote:
> >>
> >>> There are a lot of good things in this, and until the Extension bit I'm
> >>> fully on board.
> >>>
> >>> With the extension, how does the leader contender get access to the
> >>> LeaderElection? I would've assumed that LEService returns a
> >>> LeaderElection when register is called, but according to the diagram
> >>> this method doesn't return anything. Is that what
> initiateLeaderElection
> >>> is doing?
> >>>
> >>> The DefaultLeaderElection will rely on package-private methods of the
> >>> DLEService to handle confirm/hasLeadership calls?
> >>>
> >>> I don't fully understand why LContender#initializeLeaderElection is
> >>> required.
> >>>
> >>> On 05/01/2023 14:49, Matthias Pohl wrote:
> >>>> Hi everyone,
> >>>> I brought up FLINK-26522 [1] in the mailing list discussion about
> >>>> consolidating the HighAvailabilityServices interfaces [2], previously.
> >>>> There, it was concluded that the community still wants the ability to
> >>> have
> >>>> per-component leader election and, therefore, keep the
> >>>> HighAvailabilityServices interface as is. I went back to work on
> >>>> FLINK-26522 [1] to figure out how we can simplify the current codebase
> >>>> keeping the decision in mind.
> >>>>
> >>>> I wanted to handle FLINK-26522 [1] as a follow-up cleanup task of
> >>>> FLINK-24038 [3]. But while working on it, I realized that even
> >>> FLINK-24038
> >>>> [3] shouldn't have been handled without a FLIP. The per-process leader
> >>>> election which was introduced in FLINK-24038 [3] changed the ownership
> >>> of
> >>>> certain components. This is actually a change that should have been
> >>>> discussed in the mailing list and deserved a FLIP. To overcome this
> >>>> shortcoming of FLINK-24038 [3], I decided to prepare FLIP-285 [4] to
> >>>> provide proper documentation of what happened in FLINK-24038 and what
> >>> will
> >>>> be manifested with resolving its follow-up FLINK-26522 [1].
> >>>>
> >>>> Conceptually, this FLIP proposes moving away from Flink's support for
> >>>> single-contender LeaderElectionServices and introducing
> multi-contender
> >>>> support by disconnecting the HA-backend leader election lifecycle from
> >>> the
> >>>> LeaderContender's lifecycle. This allows us to provide LeaderElection
> >>> per
> >>>> component (as it was requested in [2]) but also enables us to utilize
> a
> >>>> single leader election for multiple components/contenders as well
> >>> without
> >>>> the complexity of the code that was introduced by FLINK-24038 [3].
> >>>>
> >>>> I'm looking forward to your comments.
> >>>>
> >>>> Matthias
> >>>>
> >>>> [1] https://issues.apache.org/jira/browse/FLINK-26522
> >>>> [2] https://lists.apache.org/thread/9oy2ml9s3j1v6r77h31sndhc3gw57cfm
> >>>> [3] https://issues.apache.org/jira/browse/FLINK-24038
> >>>> [4]
> >>>>
> >>>
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
> >>>
>
>

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Chesnay Schepler <ch...@apache.org>.

Thanks for updating the design. From my side this looks good.

On 18/01/2023 17:59, Matthias Pohl wrote:
> After another round of discussion, I came up with a (hopefully) final
> proposal. The previously discussed approach was still not optimal because
> the contender ID lived in the LeaderContender even though it is actually
> LeaderElectionService-internal knowledge. Fixing that helped fix the
> overall architecture. Additionally, it brought back the LeaderElection
> interface with slightly different methods.
>
> I updated the "Code Cleanup: Merge MultipleComponentLeaderElectionService
> into LeaderElectionService" section and moved the old proposal into the
> section for rejected alternatives. Feel free to have another look at the
> updated version [1].
>
> Matthias
>
> [1]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
>
> On Wed, Jan 18, 2023 at 1:40 PM Matthias Pohl <ma...@aiven.io>
> wrote:
>
>> Thanks for participating in the discussion, Yang & Chesnay. LeaderElection
>> interface extension gave me a headache as well. I added it initially
>> because I thought it would be of more value. But essentially, it doesn't
>> help but make the code harder to understand (as your questions rightfully
>> point out). I agree that the FLIP is good enough without this extension. I
>> moved it into the Rejected Alternatives section of the FLIP and would
>> propose going ahead without it.
>>
>> I will answer your questions about the LeaderElection extension, anyway:
>>
>> BTW, if the *LeaderElectionService#register(return LeaderElection)* and
>>> *LeaderElectionService#onGrantLeadership* are guarded by a same lock, then
>>> we could ensure that the leaderElection in *LeaderContender* is always
>>> non-null when it tries to confirm the leadership. And then we do not need
>>> the
>>> *LeaderContender#initializeLeaderElection*. Right?
>>
>> No, we still would need LeaderContender#initializeLeaderElection because
>> the LeaderElectionService needs to be capable of setting the LeaderElection
>> within the LeaderContender before triggering the process for granting the
>> leadership. This all needs to happen within the
>> LeaderElectionService#register(LeaderContender). It's indepent of the lock.
>>
>> With the extension, how does the leader contender get access to the
>>> LeaderElection? I would've assumed that LEService returns a LeaderElection
>>> when register is called, but according to the diagram this method doesn't
>>> return anything. Is that what initiateLeaderElection is doing?
>>
>> Correct. My initial plan was to make
>> LeaderElectionService#register(LeaderContender) return the LeaderElection
>> instance. That method could have been called within the LeaderContender.
>> But this approach has the flaw that LeaderContender would be in charge
>> within this control flow where, actually, we would want
>> LeaderElectionService to be still in charge to trigger the process for
>> granting the leadership. This required the
>> LeaderContender.initializeLeaderElection(LeaderElection) method to be added
>> to enable the LeaderElectionService to do the initialization. I added a
>> comment to the corresponding class diagram to make this clearer.
>>
>> The DefaultLeaderElection will rely on package-private methods of the
>>> DLEService to handle confirm/hasLeadership calls?
>>
>> Correct. I added the missing package-private methods to the class diagram
>> in the FLIP to clear things up.
>>
>> On Wed, Jan 18, 2023 at 11:47 AM Chesnay Schepler <ch...@apache.org>
>> wrote:
>>
>>> There are a lot of good things in this, and until the Extension bit I'm
>>> fully on board.
>>>
>>> With the extension, how does the leader contender get access to the
>>> LeaderElection? I would've assumed that LEService returns a
>>> LeaderElection when register is called, but according to the diagram
>>> this method doesn't return anything. Is that what initiateLeaderElection
>>> is doing?
>>>
>>> The DefaultLeaderElection will rely on package-private methods of the
>>> DLEService to handle confirm/hasLeadership calls?
>>>
>>> I don't fully understand why LContender#initializeLeaderElection is
>>> required.
>>>
>>> On 05/01/2023 14:49, Matthias Pohl wrote:
>>>> Hi everyone,
>>>> I brought up FLINK-26522 [1] in the mailing list discussion about
>>>> consolidating the HighAvailabilityServices interfaces [2], previously.
>>>> There, it was concluded that the community still wants the ability to
>>> have
>>>> per-component leader election and, therefore, keep the
>>>> HighAvailabilityServices interface as is. I went back to work on
>>>> FLINK-26522 [1] to figure out how we can simplify the current codebase
>>>> keeping the decision in mind.
>>>>
>>>> I wanted to handle FLINK-26522 [1] as a follow-up cleanup task of
>>>> FLINK-24038 [3]. But while working on it, I realized that even
>>> FLINK-24038
>>>> [3] shouldn't have been handled without a FLIP. The per-process leader
>>>> election which was introduced in FLINK-24038 [3] changed the ownership
>>> of
>>>> certain components. This is actually a change that should have been
>>>> discussed in the mailing list and deserved a FLIP. To overcome this
>>>> shortcoming of FLINK-24038 [3], I decided to prepare FLIP-285 [4] to
>>>> provide proper documentation of what happened in FLINK-24038 and what
>>> will
>>>> be manifested with resolving its follow-up FLINK-26522 [1].
>>>>
>>>> Conceptually, this FLIP proposes moving away from Flink's support for
>>>> single-contender LeaderElectionServices and introducing multi-contender
>>>> support by disconnecting the HA-backend leader election lifecycle from
>>> the
>>>> LeaderContender's lifecycle. This allows us to provide LeaderElection
>>> per
>>>> component (as it was requested in [2]) but also enables us to utilize a
>>>> single leader election for multiple components/contenders as well
>>> without
>>>> the complexity of the code that was introduced by FLINK-24038 [3].
>>>>
>>>> I'm looking forward to your comments.
>>>>
>>>> Matthias
>>>>
>>>> [1] https://issues.apache.org/jira/browse/FLINK-26522
>>>> [2] https://lists.apache.org/thread/9oy2ml9s3j1v6r77h31sndhc3gw57cfm
>>>> [3] https://issues.apache.org/jira/browse/FLINK-24038
>>>> [4]
>>>>
>>> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
>>>

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Matthias Pohl <ma...@aiven.io.INVALID>.

After another round of discussion, I came up with a (hopefully) final
proposal. The previously discussed approach was still not optimal because
the contender ID lived in the LeaderContender even though it is actually
LeaderElectionService-internal knowledge. Fixing that helped fix the
overall architecture. Additionally, it brought back the LeaderElection
interface with slightly different methods.

I updated the "Code Cleanup: Merge MultipleComponentLeaderElectionService
into LeaderElectionService" section and moved the old proposal into the
section for rejected alternatives. Feel free to have another look at the
updated version [1].

Matthias

[1]
https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box

On Wed, Jan 18, 2023 at 1:40 PM Matthias Pohl <ma...@aiven.io>
wrote:

> Thanks for participating in the discussion, Yang & Chesnay. LeaderElection
> interface extension gave me a headache as well. I added it initially
> because I thought it would be of more value. But essentially, it doesn't
> help but make the code harder to understand (as your questions rightfully
> point out). I agree that the FLIP is good enough without this extension. I
> moved it into the Rejected Alternatives section of the FLIP and would
> propose going ahead without it.
>
> I will answer your questions about the LeaderElection extension, anyway:
>
> BTW, if the *LeaderElectionService#register(return LeaderElection)* and
>> *LeaderElectionService#onGrantLeadership* are guarded by a same lock, then
>> we could ensure that the leaderElection in *LeaderContender* is always
>> non-null when it tries to confirm the leadership. And then we do not need
>> the
>> *LeaderContender#initializeLeaderElection*. Right?
>
>
> No, we still would need LeaderContender#initializeLeaderElection because
> the LeaderElectionService needs to be capable of setting the LeaderElection
> within the LeaderContender before triggering the process for granting the
> leadership. This all needs to happen within the
> LeaderElectionService#register(LeaderContender). It's indepent of the lock.
>
> With the extension, how does the leader contender get access to the
>> LeaderElection? I would've assumed that LEService returns a LeaderElection
>> when register is called, but according to the diagram this method doesn't
>> return anything. Is that what initiateLeaderElection is doing?
>
>
> Correct. My initial plan was to make
> LeaderElectionService#register(LeaderContender) return the LeaderElection
> instance. That method could have been called within the LeaderContender.
> But this approach has the flaw that LeaderContender would be in charge
> within this control flow where, actually, we would want
> LeaderElectionService to be still in charge to trigger the process for
> granting the leadership. This required the
> LeaderContender.initializeLeaderElection(LeaderElection) method to be added
> to enable the LeaderElectionService to do the initialization. I added a
> comment to the corresponding class diagram to make this clearer.
>
> The DefaultLeaderElection will rely on package-private methods of the
>> DLEService to handle confirm/hasLeadership calls?
>
>
> Correct. I added the missing package-private methods to the class diagram
> in the FLIP to clear things up.
>
> On Wed, Jan 18, 2023 at 11:47 AM Chesnay Schepler <ch...@apache.org>
> wrote:
>
>> There are a lot of good things in this, and until the Extension bit I'm
>> fully on board.
>>
>> With the extension, how does the leader contender get access to the
>> LeaderElection? I would've assumed that LEService returns a
>> LeaderElection when register is called, but according to the diagram
>> this method doesn't return anything. Is that what initiateLeaderElection
>> is doing?
>>
>> The DefaultLeaderElection will rely on package-private methods of the
>> DLEService to handle confirm/hasLeadership calls?
>>
>> I don't fully understand why LContender#initializeLeaderElection is
>> required.
>>
>> On 05/01/2023 14:49, Matthias Pohl wrote:
>> > Hi everyone,
>> > I brought up FLINK-26522 [1] in the mailing list discussion about
>> > consolidating the HighAvailabilityServices interfaces [2], previously.
>> > There, it was concluded that the community still wants the ability to
>> have
>> > per-component leader election and, therefore, keep the
>> > HighAvailabilityServices interface as is. I went back to work on
>> > FLINK-26522 [1] to figure out how we can simplify the current codebase
>> > keeping the decision in mind.
>> >
>> > I wanted to handle FLINK-26522 [1] as a follow-up cleanup task of
>> > FLINK-24038 [3]. But while working on it, I realized that even
>> FLINK-24038
>> > [3] shouldn't have been handled without a FLIP. The per-process leader
>> > election which was introduced in FLINK-24038 [3] changed the ownership
>> of
>> > certain components. This is actually a change that should have been
>> > discussed in the mailing list and deserved a FLIP. To overcome this
>> > shortcoming of FLINK-24038 [3], I decided to prepare FLIP-285 [4] to
>> > provide proper documentation of what happened in FLINK-24038 and what
>> will
>> > be manifested with resolving its follow-up FLINK-26522 [1].
>> >
>> > Conceptually, this FLIP proposes moving away from Flink's support for
>> > single-contender LeaderElectionServices and introducing multi-contender
>> > support by disconnecting the HA-backend leader election lifecycle from
>> the
>> > LeaderContender's lifecycle. This allows us to provide LeaderElection
>> per
>> > component (as it was requested in [2]) but also enables us to utilize a
>> > single leader election for multiple components/contenders as well
>> without
>> > the complexity of the code that was introduced by FLINK-24038 [3].
>> >
>> > I'm looking forward to your comments.
>> >
>> > Matthias
>> >
>> > [1] https://issues.apache.org/jira/browse/FLINK-26522
>> > [2] https://lists.apache.org/thread/9oy2ml9s3j1v6r77h31sndhc3gw57cfm
>> > [3] https://issues.apache.org/jira/browse/FLINK-24038
>> > [4]
>> >
>> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
>> >
>>
>>

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Matthias Pohl <ma...@aiven.io.INVALID>.

Thanks for participating in the discussion, Yang & Chesnay. LeaderElection
interface extension gave me a headache as well. I added it initially
because I thought it would be of more value. But essentially, it doesn't
help but make the code harder to understand (as your questions rightfully
point out). I agree that the FLIP is good enough without this extension. I
moved it into the Rejected Alternatives section of the FLIP and would
propose going ahead without it.

I will answer your questions about the LeaderElection extension, anyway:

BTW, if the *LeaderElectionService#register(return LeaderElection)* and
> *LeaderElectionService#onGrantLeadership* are guarded by a same lock, then
> we could ensure that the leaderElection in *LeaderContender* is always
> non-null when it tries to confirm the leadership. And then we do not need
> the
> *LeaderContender#initializeLeaderElection*. Right?

No, we still would need LeaderContender#initializeLeaderElection because
the LeaderElectionService needs to be capable of setting the LeaderElection
within the LeaderContender before triggering the process for granting the
leadership. This all needs to happen within the
LeaderElectionService#register(LeaderContender). It's indepent of the lock.

With the extension, how does the leader contender get access to the
> LeaderElection? I would've assumed that LEService returns a LeaderElection
> when register is called, but according to the diagram this method doesn't
> return anything. Is that what initiateLeaderElection is doing?

Correct. My initial plan was to make
LeaderElectionService#register(LeaderContender) return the LeaderElection
instance. That method could have been called within the LeaderContender.
But this approach has the flaw that LeaderContender would be in charge
within this control flow where, actually, we would want
LeaderElectionService to be still in charge to trigger the process for
granting the leadership. This required the
LeaderContender.initializeLeaderElection(LeaderElection) method to be added
to enable the LeaderElectionService to do the initialization. I added a
comment to the corresponding class diagram to make this clearer.

The DefaultLeaderElection will rely on package-private methods of the
> DLEService to handle confirm/hasLeadership calls?

Correct. I added the missing package-private methods to the class diagram
in the FLIP to clear things up.

On Wed, Jan 18, 2023 at 11:47 AM Chesnay Schepler <ch...@apache.org>
wrote:

> There are a lot of good things in this, and until the Extension bit I'm
> fully on board.
>
> With the extension, how does the leader contender get access to the
> LeaderElection? I would've assumed that LEService returns a
> LeaderElection when register is called, but according to the diagram
> this method doesn't return anything. Is that what initiateLeaderElection
> is doing?
>
> The DefaultLeaderElection will rely on package-private methods of the
> DLEService to handle confirm/hasLeadership calls?
>
> I don't fully understand why LContender#initializeLeaderElection is
> required.
>
> On 05/01/2023 14:49, Matthias Pohl wrote:
> > Hi everyone,
> > I brought up FLINK-26522 [1] in the mailing list discussion about
> > consolidating the HighAvailabilityServices interfaces [2], previously.
> > There, it was concluded that the community still wants the ability to
> have
> > per-component leader election and, therefore, keep the
> > HighAvailabilityServices interface as is. I went back to work on
> > FLINK-26522 [1] to figure out how we can simplify the current codebase
> > keeping the decision in mind.
> >
> > I wanted to handle FLINK-26522 [1] as a follow-up cleanup task of
> > FLINK-24038 [3]. But while working on it, I realized that even
> FLINK-24038
> > [3] shouldn't have been handled without a FLIP. The per-process leader
> > election which was introduced in FLINK-24038 [3] changed the ownership of
> > certain components. This is actually a change that should have been
> > discussed in the mailing list and deserved a FLIP. To overcome this
> > shortcoming of FLINK-24038 [3], I decided to prepare FLIP-285 [4] to
> > provide proper documentation of what happened in FLINK-24038 and what
> will
> > be manifested with resolving its follow-up FLINK-26522 [1].
> >
> > Conceptually, this FLIP proposes moving away from Flink's support for
> > single-contender LeaderElectionServices and introducing multi-contender
> > support by disconnecting the HA-backend leader election lifecycle from
> the
> > LeaderContender's lifecycle. This allows us to provide LeaderElection per
> > component (as it was requested in [2]) but also enables us to utilize a
> > single leader election for multiple components/contenders as well without
> > the complexity of the code that was introduced by FLINK-24038 [3].
> >
> > I'm looking forward to your comments.
> >
> > Matthias
> >
> > [1] https://issues.apache.org/jira/browse/FLINK-26522
> > [2] https://lists.apache.org/thread/9oy2ml9s3j1v6r77h31sndhc3gw57cfm
> > [3] https://issues.apache.org/jira/browse/FLINK-24038
> > [4]
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
> >
>
>

Re: [DISCUSS] FLIP-285: Refactoring the leader election code base in Flink

Posted by Chesnay Schepler <ch...@apache.org>.

There are a lot of good things in this, and until the Extension bit I'm 
fully on board.

With the extension, how does the leader contender get access to the 
LeaderElection? I would've assumed that LEService returns a 
LeaderElection when register is called, but according to the diagram 
this method doesn't return anything. Is that what initiateLeaderElection 
is doing?

The DefaultLeaderElection will rely on package-private methods of the 
DLEService to handle confirm/hasLeadership calls?

I don't fully understand why LContender#initializeLeaderElection is 
required.

On 05/01/2023 14:49, Matthias Pohl wrote:
> Hi everyone,
> I brought up FLINK-26522 [1] in the mailing list discussion about
> consolidating the HighAvailabilityServices interfaces [2], previously.
> There, it was concluded that the community still wants the ability to have
> per-component leader election and, therefore, keep the
> HighAvailabilityServices interface as is. I went back to work on
> FLINK-26522 [1] to figure out how we can simplify the current codebase
> keeping the decision in mind.
>
> I wanted to handle FLINK-26522 [1] as a follow-up cleanup task of
> FLINK-24038 [3]. But while working on it, I realized that even FLINK-24038
> [3] shouldn't have been handled without a FLIP. The per-process leader
> election which was introduced in FLINK-24038 [3] changed the ownership of
> certain components. This is actually a change that should have been
> discussed in the mailing list and deserved a FLIP. To overcome this
> shortcoming of FLINK-24038 [3], I decided to prepare FLIP-285 [4] to
> provide proper documentation of what happened in FLINK-24038 and what will
> be manifested with resolving its follow-up FLINK-26522 [1].
>
> Conceptually, this FLIP proposes moving away from Flink's support for
> single-contender LeaderElectionServices and introducing multi-contender
> support by disconnecting the HA-backend leader election lifecycle from the
> LeaderContender's lifecycle. This allows us to provide LeaderElection per
> component (as it was requested in [2]) but also enables us to utilize a
> single leader election for multiple components/contenders as well without
> the complexity of the code that was introduced by FLINK-24038 [3].
>
> I'm looking forward to your comments.
>
> Matthias
>
> [1] https://issues.apache.org/jira/browse/FLINK-26522
> [2] https://lists.apache.org/thread/9oy2ml9s3j1v6r77h31sndhc3gw57cfm
> [3] https://issues.apache.org/jira/browse/FLINK-24038
> [4]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-285%3A+Refactoring+LeaderElection+to+make+Flink+support+multi-component+leader+election+out-of-the-box
>