Leader Election #523 #538

atttx123 · 2019-03-27T09:24:29Z

leader election #523
@yue9944882 @brendandburns

by the way, i'm not sure how to write the unit test code for the leader election.

k8s-ci-robot · 2019-03-27T09:24:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: atttx123
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mbohlool

If they are not already assigned, you can assign the PR to them by writing /assign @mbohlool in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-03-27T09:24:41Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

yue9944882

thank you for contributing this!

i left some comments and will try running/testing the implementation later in my local environment

yue9944882 · 2019-03-27T12:09:31Z

util/src/main/java/io/kubernetes/client/leaderelection/resourcelock/ResourceLockConfig.java

+    /**
+     * EventRecorder is optional.
+     */
+    private EventRecorder eventRecorder;


remove this. we should design/implement event utilities in another pull. i updated the issue to reflect the task.

EventRecorderis just a interface for ResourceLock to record the event,
client-go/leaderelection/resroucelock/interface.go,

don't design it if it doesn't intend to use it

OK, i will delete this interface.

yue9944882 · 2019-03-27T12:15:04Z

util/src/main/java/io/kubernetes/client/leaderelection/resourcelock/ResourceLock.java

+    }
+
+    private String getRootUrl() {
+        return String.format("/api/%s/namespaces/%s/%s",


we're introducing coordination api in the future releases, so it doesn't have to be a legacy/api path. dealing w/ format printing url here doesn't make sense. better make it an interface or lambda returning an (generic?) object

Good idea, i will change this and add more unit test, it will take a time to complete

there is a not good design in k8s-java-client, because all k8s generated resource type do not have an common parent class. if ConfigMap and Endpoint extends HashedMetaObject, then this interface will be more meaningful.

if ConfigMap and Endpoint extends HashedMetaObject, then this interface will be more meaningful.

we have limitted kinds of resources for leader-electing. i presume just multiple independant resource lock classes is fine. separated class like ConfigMapResourceLock, EndpointsResourceLock, CoordinationResourceLock..

@brendandburns for thoughts

yue9944882 · 2019-03-27T12:16:01Z

util/src/main/java/io/kubernetes/client/leaderelection/resourcelock/ResourceLock.java

+        this.name = name;
+        this.namespace = namespace;
+        this.client = client;
+        this.apiVersion = apiVersion;


we don't have a second version for /api since that it's a legacy path

yue9944882 · 2019-03-27T12:31:25Z

examples/src/main/java/io/kubernetes/client/examples/LeaderElectionExample.java

+        try {
+            leaderElector.run();
+        } finally {
+            leaderElector.cancel();


sorry to be dense. what's the point of cancelling if the elector.run already returned?

i just fellow the logic in leaderelection.go, the Run function behave like this:

block the call thread

if itself get the leader lock, run a goroutine for OnStartedLeading

if others get the leader lock, this lock will fail into a while sleep loop, unit itself get the leader lock
so, the leaderElector.run() will block the thread and will not return, so the cancel() function is designed for elegant exit.

cancel here seems supposed to called in another thread. what'll read the boolean if run already exit?

yes，cancel() set an AtomicBoolean like a stop channel in golang

while the example suggests that "we're supposed to call cancel after the elector exited". i'm confused about the order.

yue9944882 · 2019-03-27T12:56:28Z

util/src/test/java/io/kubernetes/client/util/leaderelection/LeaderElectionTest.java

+import java.util.concurrent.TimeUnit;
+
+/**
+ * Created by IDEA on 2019-03-27 16:06


nit: minimize docs

yue9944882 · 2019-03-27T12:59:48Z

and btw, cncf seems unhappy

/check-cla

atttx123 · 2019-03-28T04:21:13Z

I signed it

atttx123 · 2019-03-28T13:28:20Z

@yue9944882
sorry to be dense. how can i solve the Non complying file error ?

yue9944882

sorry to be dense. how can i solve the Non complying file error ?

it's the formatter unhappy. https://github.com/coveo/fmt-maven-plugin#command-line

yue9944882 · 2019-03-29T15:02:58Z

util/src/main/java/io/kubernetes/client/leaderelection/LeaderElectionRecord.java

+  private String renewTime;
+  private int leaderTransitions = 0;
+
+  public LeaderElectionRecord() {


remove this

LeaderElectionRecord means record saved in crd.metadata.annotations.

please remove the unused non-param constructor

yue9944882 · 2019-03-29T15:27:00Z

examples/src/main/java/io/kubernetes/client/examples/LeaderElectionExample.java

+        try {
+            leaderElector.run();
+        } finally {
+            leaderElector.cancel();


while the example suggests that "we're supposed to call cancel after the elector exited". i'm confused about the order.

yue9944882 · 2019-03-29T15:28:27Z

util/src/main/java/io/kubernetes/client/leaderelection/LeaderElector.java

+/**
+ * LeaderElector is a leader election client.
+ */
+public class LeaderElector implements Runnable {


what's the point of implementing runnable?

yue9944882 · 2019-03-29T15:45:53Z

util/src/main/java/io/kubernetes/client/leaderelection/LeaderElector.java

+      if (!acquire(cancel)) {
+        return;
+      }
+      Runnable runnable = () -> config.getCallbacks().onStartedLeading();


nit: make it an anonymous lambda and fold it into the following line

yue9944882 · 2019-03-29T15:52:46Z

util/src/main/java/io/kubernetes/client/leaderelection/LeaderElector.java

+      return;
+    }
+    reportedLeader = observedRecord.getHolderIdentity();
+    Callable<Boolean> callable = () -> {


nit: make it an anonymous lambda and fold it into the following line

this comment also applies here

yue9944882 · 2019-03-29T15:56:02Z

util/src/main/java/io/kubernetes/client/leaderelection/LeaderElector.java

+
+  private AtomicBoolean cancel = new AtomicBoolean(false);
+
+  private ExecutorService executor;


makes it a ScheduledExecutorService

additionally, i think we should split it into two separate pools, one for running lease and one for hooks. the latter one should be a fixed pool at size 1.

yue9944882 · 2019-03-29T16:00:29Z

util/src/main/java/io/kubernetes/client/leaderelection/LeaderElectionConfig.java

+   * LeaseDuration is the duration that non-leader candidates will wait to force acquire leadership.
+   * This is measured against time of last observed ack.
+   */
+  private long leaseDuration;


java.time.Duration looks better?

yue9944882 · 2019-03-29T16:03:45Z

util/src/main/java/io/kubernetes/client/leaderelection/LeaderElector.java

+            }
+            return done;
+          };
+          executor.invokeAll(


Suggested change

executor.invokeAll(

executor.scheduleAtFixedRate(() -> {

// ... tryAcquireOrRenew

}, 0, getRetryPeriod().toMillis(), TimeUnit.MILLISECONDS)

they are two different functions:

invokeAll(Collection<? extends Callable<T>> tasks, long timeout, TimeUnit unit) scheduleAtFixedRate(Runnable command, long initialDelay, long period, TimeUnit unit)

the logic there:

the tryAcquireOrRenew function will tried every getRetryPeriod (example 5s), and if total cost up to getRenewDeadline (example 15s) will throw InterruptedException

this will avoid dead lock or Infinite waiting in logic code

isn't it better if we retry w/ the help of a scheduled thread pool?

yue9944882 · 2019-03-29T16:06:59Z

util/src/main/java/io/kubernetes/client/leaderelection/LeaderElector.java

+   * internal bookkeeping
+   */
+  private LeaderElectionRecord observedRecord;
+  private Long observedTime;


nit: makes it either a Date or a long(built-in)

yue9944882 · 2019-03-29T16:08:35Z

util/src/main/java/io/kubernetes/client/leaderelection/LeaderElector.java

+    } catch (ResourceNotFoundException e) {
+      try {
+        config.getLock().create(leaderElectionRecord);
+      } catch (ApiException e1) {


a better name for e1. we don't like to do extra reverse engineering work, do we?

yue9944882 · 2019-07-15T11:13:14Z

/close

closing in favor of #590

k8s-ci-robot · 2019-07-15T11:13:15Z

@yue9944882: Closed this PR.

In response to this:

/close

closing in favor of #590

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

feature: java Leader Election tools

1e6a937

k8s-ci-robot requested review from brendandburns and mbohlool March 27, 2019 09:24

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 27, 2019

yue9944882 reviewed Mar 27, 2019

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 28, 2019

atttx123 added 2 commits March 28, 2019 20:04

chore: reformat code to google style, remove IDE generated comment

f2b6778

chore: add complying to file; remove useless interface EventRecorder

a836018

yue9944882 reviewed Mar 29, 2019

View reviewed changes

k8s-ci-robot closed this Jul 15, 2019

moeyui1 mentioned this pull request Sep 19, 2019

"renewDeadline" support for leader election #706

Closed


		private AtomicBoolean cancel = new AtomicBoolean(false);

		private ExecutorService executor;

-          executor.invokeAll(
+          executor.scheduleAtFixedRate(() -> {
+              // ... tryAcquireOrRenew
+          }, 0, getRetryPeriod().toMillis(), TimeUnit.MILLISECONDS)

Leader Election #523 #538

Leader Election #523 #538

Uh oh!

Conversation

atttx123 commented Mar 27, 2019

Uh oh!

k8s-ci-robot commented Mar 27, 2019

Uh oh!

k8s-ci-robot commented Mar 27, 2019

Uh oh!

yue9944882 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atttx123 Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yue9944882 commented Mar 27, 2019

Uh oh!

atttx123 commented Mar 28, 2019

Uh oh!

atttx123 commented Mar 28, 2019

Uh oh!

yue9944882 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atttx123 Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atttx123 Mar 28, 2019 •

edited

Loading

atttx123 Apr 11, 2019 •

edited

Loading