Leader Election#1358
Conversation
csviri
force-pushed
the
next
branch
2 times, most recently
from
f6aa23f to
e3cd675
Compare
|
Note that the e2e test has to be this was because of the |
csviri
force-pushed
the
leader-election
branch
2 times, most recently
from
1d83237 to
f1b0297
Compare
csviri
force-pushed
the
leader-election
branch
from
f1b0297 to
de9d553
Compare
metacosm
left a comment
metacosm
left a comment
There was a problem hiding this comment.
Made some changes, also have some comments but looks good otherwise.
|
|
||
| public LeaderElectionConfigurationBuilder() {} | ||
|
|
||
| public static LeaderElectionConfigurationBuilder aLeaderElectionConfiguration() { |
There was a problem hiding this comment.
I don't really see the point of this builder as it is just creating the object using the publicly available constructors…
There was a problem hiding this comment.
deleted. Added an additional constructor.
| * Handles calls and results of a Reconciler and finalizer related logic | ||
| */ | ||
| class ReconciliationDispatcher<R extends HasMetadata> { | ||
| class ReconciliationDispatcher<P extends HasMetadata> { |
There was a problem hiding this comment.
It would be better to do these kind of changes in different PRs as this makes things more difficult to review
There was a problem hiding this comment.
yes, will try to do in the future.
| this.identity = identity(config); | ||
| Lock lock = new LeaseLock(config.getLeaseNamespace(), config.getLeaseName(), identity); | ||
| // releaseOnCancel is not used in the underlying implementation | ||
| leaderElector = new LeaderElectorBuilder(client, |
There was a problem hiding this comment.
This API is quite awkward, imo… maybe we should improve it in the Fabric8 client project?
There was a problem hiding this comment.
Hmm, yes, this could be better there.
|
|
||
| ExecutorServiceManager.init(); | ||
| controllers.start(); | ||
| // first start the controller manager before leader election, |
There was a problem hiding this comment.
This is a bit smelly, imo. Would it make sense to put the election-related behavior in the ControllerManager to simplify the state management?
There was a problem hiding this comment.
It has nothing to do with the controllers, it's more like a logic regarding the whole operator. Not saying that it is ideally describing the domain, but I don't think it should be in the controller. Controllers actually should not know if there is such thing as leader election in the background.
| .inNamespace(namespace).withName(TEST_RESOURCE_NAME).get().getStatus(); | ||
|
|
||
| assertThat(actualStatus).isNotNull(); | ||
| assertThat(actualStatus.getReconciledBy()).hasSizeGreaterThan(actualListSize + 2); |
There was a problem hiding this comment.
Why +2? Why not use a set on the status so that we can check that both instances have reconciled the resource?
There was a problem hiding this comment.
Added a named constant
|
|
||
| public Optional<String> getIdentity() { | ||
| return Optional.ofNullable(identity); | ||
| public String getIdentity() { |
There was a problem hiding this comment.
TBH I don't think this is good, would like to keep the POJOs clear without any logic
csviri
force-pushed
the
leader-election
branch
from
79b4b6b to
951d28d
Compare
This reverts commit 93a260a.
|
Kudos, SonarCloud Quality Gate passed! |
Fixes #411 Co-authored-by: Chris Laprun <metacosm@gmail.com>
|
Very much looking forward to this! Thanks for implementing it. |
Fixes #411 Co-authored-by: Chris Laprun <metacosm@gmail.com>
Fixes #411 Co-authored-by: Chris Laprun <metacosm@gmail.com>
3 participants
No description provided.