Refactor/Clean leader election related classes #5937
Conversation
|
I am trying to see if we can use the already existing leader logic/classes from fabric8 in spring-cloud-kubernetes. There is already an implementation there, but may be we can refactor and use what you folks already have in place. ' As such, I am trying to poke a little into the code base to understand how things are done, thus this PR with minor clean-ups. Thank you for looking into it |
|
I will mark this one as ready after the build passes, but in order to do that, someone has to approve that, so that the build starts |
| private Runnable onStartLeading; | ||
| private Runnable onStopLeading; | ||
| private Consumer<String> onNewLeader; | ||
| private final Runnable onStartLeading; |
There was a problem hiding this comment.
just add final, nothing more changes in this class
| @@ -46,10 +45,9 @@ public class LeaderElector { | |||
|
|
|||
| protected static final Double JITTER_FACTOR = 1.2; | |||
|
|
|||
| private KubernetesClient kubernetesClient; | |||
| private LeaderElectionConfig leaderElectionConfig; | |||
| private final KubernetesClient kubernetesClient; | |||
| private final AtomicReference<LeaderElectionRecord> observedRecord = new AtomicReference<>(); | ||
| private final AtomicReference<LocalDateTime> observedTime = new AtomicReference<>(); |
There was a problem hiding this comment.
I dropped the observedTime variable, it was updated, but never used
There was a problem hiding this comment.
I'm not sure why this was added on the first place. I need to do some digging before we can merge this.
There was a problem hiding this comment.
The go client switch how time was tracked so that it was local to each elector, but we never fully implemented that.
There was a problem hiding this comment.
I traced it back to the initial implementation but it was never used. I guess I left that there for a follow up.
|
@manusa can I ask please to re-start the pipeline? |
|
hmm, there is a failing test, but its not related to these changes. I took a look at it, and it of course passes locally ( |
|
wind57
requested review from
manusa,
oscerd,
rohanKanojia,
shawkins and
sunix
as code owners
|
@manusa asking for your input please here, I can keep this one up to date with |
|
@wind57 : Usually we require 2 approvals for a pull request to get merged. Most people working on these projects are also working on other projects, so it might take a week or two for pull request to get merged. |
No description provided.