xintongsong commented on PR #21137:
URL: https://github.com/apache/flink/pull/21137#issuecomment-1301899690
> If you look at the code without context you now have 2 arbitrary calls go
through an executor and you make similar decisions as with locking ("what
operations should run in the executor").
It would be ideal if `JobMasterServiceLeadershipRunner` need not be aware of
the lock inside the leader election service. Unfortunately, I don't see a good
way to do so.
In the current approach, the name of the executor clearly states that it is
supposed to be used for handling the leadership related events, which are
granting and revoking. You can safely access the runner lock in any other
methods of the runner, without worrying about causing deadlocks with another
lock from another class. Otherwise, imagine you called a method `a()` in
`grantLeadership()`, and `b()` in `a()`, and `c()` in `b()`, etc. One can
easily get confused whether `c()` is called under the leader election service
lock and whether it is safe to access the runner lock. That is to explain why I
think the current approach is good for the maintenance.
>The javadocs states that all operations are serialized, yet we need an
executor for certain operations to make things work. That's just an accident
waiting to happen.
I don't get what do you mean by an accident waiting to happen. The JavaDoc
says the granting and revoking of leadership are serialized, not the other
public methods of the class.
> calling grant/revokeLeadership no longer has an immediate effect (as it
now effectively runs in the background)
Exactly. The change is based on the fact that the runner neither need to do
the works synchronously in grant/revokeLeadership, nor need/want to be aware of
the lock that the leader election service put upon the two methods.
> I'd rather see us pushing more code into the sequentialOperation and
always running that in the executor
Well, that should also work. I don't see much differences compare the
current approach, except for more code changes. But I'm not opposed to it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
