add etcd watch implementation #45

StupidHod · 2016-08-30T07:53:47Z

With the wath design, I have finished the watch implementation, hope for your review.
@adohe @xiang90

adohe-zz · 2016-08-30T23:33:23Z

Good job @StupidHod , will take a look asap.

oza · 2016-08-31T10:38:41Z

src/main/java/com/coreos/jetcd/EtcdWatch.java

+import com.coreos.jetcd.options.WatchOption;
+import com.coreos.jetcd.util.ListenableSetFuture;
+import com.coreos.jetcd.watch.Watcher;
+import com.google.protobuf.ByteString;


IMHO, it's better to avoid exposing ByteString directly since Apache Felix's iPOJO abstraction can be broken. Let me explain the detail. protobuf compiler generates non-compatible source code(I mean it's source code-level compatibility, not wire-level compatibility). If a user of jetcd tries to use another protobuf compiler(e.g. v2.5.0 compiler) for her applications, it can happen for users to pass non-compatible(v2.5.0) ByteString bytecode. My suggestion is to create a wrapper class of ByteString for preventing users from touching jetcd's ByteString.

@oza thanks for your great suggestion. If we expose java String to users, and convert it to ByteString, wdyt?

One concern to use Java String directly is a semantic gap between Golang's string and Java's string. IIUC, etcd's key can contain illegal String for java String since Golang's string is just a wrapper of byte[]. As a result, it can happen to fail to decode as Java's string. It's not trivial to handle them correctly at jetcd layer. My suggestion is to create primitive APIs which accept/return byte[], and create utility API which convert byte[] in primitive APIs to Java String on the top of them.

@xiang90 could you confirm this? If so we have to wrap this.

@oza is correct.

oza · 2016-08-31T11:04:21Z

Hi @StupidHod, thanks for your great job! I'm interested in jetcd, so I'm commenting here. I checked some points about API-level comment and initialization sequence of RequestStream.

xiang90 · 2016-09-01T13:47:35Z

@oza Great news! Would love to have you contribute more to jetcd! Thanks a lot for the help so far!

oza · 2016-09-02T00:32:52Z

@xiang90 Happy to hear that :-) Yes, I will contribute more to jetcd!

adohe-zz · 2016-09-02T10:58:41Z

@StupidHod @oza I would support we construct kind of readable/writable byte sequence, we can name it ByteSequence, this ByteSequence can accpet/return byte[], also can accept string, and if possible we can return string.

lburgazzoli · 2016-09-02T11:08:49Z

ByteSequence should also accept CharSequence

StupidHod · 2016-09-03T01:42:34Z

@adohe @lburgazzoli Got your advices.

lburgazzoli · 2016-09-03T09:45:40Z

No it is a lazy singleton

adohe-zz · 2016-09-03T13:16:24Z

src/main/java/com/coreos/jetcd/EtcdWatchImpl.java

+
+    private ConcurrentHashMap<Long, Watcher> watchers = new ConcurrentHashMap<>();
+
+    private WatchGrpc.WatchStub watchStub;


private final could be better.

Ok, got it.

StupidHod · 2016-09-05T09:25:11Z

As the discussion, I have done some changes as below.

Change some sync detail, such as remove sync on watch function.
Split watcher in Watcher(interface in EtcdWatch) and WatcherImpl(inner class in EtcdWatchImpl), adviced by @lburgazzoli .
Add cancel function for Watcher, return a CompletableFuture result, adviced by @lburgazzoli ..
Add ByteSequence to replace ByteString, discussed in Shading Guava. #47 .
Add Etcd Client KeyValue to replace gRPC KeyValue.
Add Etcd Client Header to replace gRPC Response Header.
Add EtcdUtil to convert gRPC object to EtcdClient Object.
Replace ListableFuture with CompletableFuture, discussed in EtcdLease should not use ListenableFuture directly #46 .

lburgazzoli · 2016-09-05T10:45:49Z

src/main/java/com/coreos/jetcd/EtcdClient.java

        }

        this.kvClient = newKVClient(kvStub);
        this.authClient = newAuthClient(authStub);
        this.maintenanceClient = newMaintenanceClient(mainFStub, mainStub);
        this.clusterClient = newClusterClient(clusterStub);
+        this.watchClient = newWatchClient(watchStub);
+


It is really needed to create so many objects bu default ?
I would instead use Suppliers.memorize or any other way to have lazy object creation

adohe-zz · 2016-09-06T02:35:58Z

@StupidHod @oza @lburgazzoli I think this is just what we want. All you guys are amazing 👍 For this PR I would hope we can focus on the thread safety, I would like to implement something quite clean but enough safe.

StupidHod · 2016-09-06T02:41:04Z

Yes, for some points, we can implement them later.

oza · 2016-09-06T15:23:23Z

OK, checking thread safety.

oza · 2016-09-06T23:30:17Z

src/main/java/com/coreos/jetcd/EtcdWatchImpl.java

+
+        WatchCancelRequest cancelRequest = WatchCancelRequest.newBuilder().setWatchId(id).build();
+        WatchRequest request = WatchRequest.newBuilder().setCancelRequest(cancelRequest).build();
+        this.requestStream.onNext(request);


Direct usage of requestStream can be null without lock. getRequestStream() should be called instead.

Thanks very much.

sundbp · 2016-09-19T21:48:32Z

Curious if this will get merged soon? watch implementation and a release of jetcd being available would be great!

adohe-zz · 2016-09-20T00:04:55Z

@StupidHod @oza @lburgazzoli let's check what still remaining, we should get this merge asap.

StupidHod · 2016-09-23T03:01:31Z

@adohe I think you are right, I have fixed the issues proposed, hope for your advices.

abhin4v · 2016-10-18T08:38:53Z

@adohe Will this PR get merged soon? Last comment on this was almost a month ago.

adohe-zz · 2016-10-18T10:54:39Z

@abhin4v I will recheck this, and believe we can merge this before this weekend.

blackgwe · 2016-11-07T16:11:31Z

@xiang90 Any show stopper for this PR?

adohe-zz · 2016-11-07T23:58:04Z

@blackgwe I will recheck this with @StupidHod, will get this merged asap.

StupidHod · 2016-11-08T02:52:51Z

OK, got it. @adohe

forif · 2016-12-02T09:00:16Z

We now urge for the java watcher service, hope this PR get merged soon.

xiang90 · 2016-12-02T17:09:42Z

@adohe Can you please take another look?

krestenkrab · 2017-03-15T13:29:21Z

src/main/java/com/coreos/jetcd/EtcdWatchImpl.java

+import com.google.protobuf.ByteString;
+import io.grpc.ManagedChannel;
+import io.grpc.stub.StreamObserver;
+import javafx.util.Pair;


You should not include javafx as a dependency; nothing else in this project does. It also means this only works with Oracle JDKs (i.e. it won't work with OpenJDK). If need be, add a package-local class at the end of the file like this:

class Pair<K,V> { final K k; final V v; Pair(K k, V v) { this.k = k; this.v = v; } K getKey() { return k; } V getValue() { return v; } @Override public int hashCode() { return k.hashCode() + v.hashCode(); } @Override public boolean equals(Object obj) { if (obj==null || !(obj instanceof Pair)) return false; Pair<K,V> other = (Pair<K,V>)obj; return k.equals(other.k) && v.equals(other.v); } }

This comment applies for the https://github.com/enncloud/jetcd changes too. I use the Zulu JVM by the way, which is a maintained branch of OpenJDK by the fine folks at Azul.

/cc @enncloud.

krestenkrab · 2017-03-15T13:40:21Z

src/main/java/com/coreos/jetcd/EtcdWatchImpl.java

+     */
+    private synchronized void resume() {
+        this.requestStream = null;
+        WatcherImpl[] resumeWatchers = (WatcherImpl[]) watchers.values().toArray();


This should be

WatcherImpl[] resumeWatchers = (WatcherImpl[]) watchers.values().toArray(new WatcherImpl[watchers.size()]);

or you will get a class cast exception

StupidHod · 2017-03-17T22:42:44Z

@krestenkrab Thank you for your reviewing. I will fix them soon.

Signed-off-by: Steven M. Pritko <[email protected]>

oza reviewed Aug 31, 2016
View reviewed changes

oza mentioned this pull request Sep 1, 2016

EtcdLease should not use ListenableFuture directly #46

Closed

This was referenced Sep 1, 2016

Client interfaces #16

Closed

Shading Guava. #47

Closed

adohe-zz reviewed Sep 3, 2016
View reviewed changes

lburgazzoli reviewed Sep 5, 2016
View reviewed changes

oza reviewed Sep 6, 2016
View reviewed changes

StupidHod force-pushed the watch branch from ad661f4 to 5029a0f Compare September 8, 2016 06:43

StupidHod force-pushed the watch branch from 5029a0f to 1b8d55d Compare October 27, 2016 02:09

adohe-zz mentioned this pull request Nov 10, 2016

Code cleanup #53

Merged

StupidHod added 5 commits December 2, 2016 11:29

add etcd watch implementation

c5b17e8

change gRPC class to package class

177de5b

add final for watch stub

8d2baa5

add closeable for Watcher, change visible for EtcdUtil

5e16176

change EtcdWatchImpl constract interface

20c19b5

StupidHod force-pushed the watch branch from 1b8d55d to 20c19b5 Compare December 2, 2016 04:11

fanminshi mentioned this pull request Mar 14, 2017

discuss watch client #42

Closed

krestenkrab reviewed Mar 15, 2017

View reviewed changes

spritko pushed a commit to spritko/jetcd that referenced this pull request Apr 26, 2017

Add watch support from PR etcd-io#45

ad7d2e1

Signed-off-by: Steven M. Pritko <[email protected]>

fanminshi mentioned this pull request Apr 26, 2017

watch: fix style warnings and minor nits for StupidHod's watch implementation. #84

Merged

fanminshi merged commit 20c19b5 into etcd-io:master Apr 26, 2017


		private ConcurrentHashMap<Long, Watcher> watchers = new ConcurrentHashMap<>();

		private WatchGrpc.WatchStub watchStub;

add etcd watch implementation #45

add etcd watch implementation #45

Conversation

StupidHod commented Aug 30, 2016

adohe-zz commented Aug 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oza commented Aug 31, 2016

xiang90 commented Sep 1, 2016

oza commented Sep 2, 2016

adohe-zz commented Sep 2, 2016

lburgazzoli commented Sep 2, 2016

StupidHod commented Sep 3, 2016

lburgazzoli commented Sep 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StupidHod commented Sep 5, 2016 • edited Loading

Choose a reason for hiding this comment

adohe-zz commented Sep 6, 2016

StupidHod commented Sep 6, 2016

oza commented Sep 6, 2016

oza Sep 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sundbp commented Sep 19, 2016

adohe-zz commented Sep 20, 2016

StupidHod commented Sep 23, 2016

abhin4v commented Oct 18, 2016

adohe-zz commented Oct 18, 2016

blackgwe commented Nov 7, 2016

adohe-zz commented Nov 7, 2016

StupidHod commented Nov 8, 2016

forif commented Dec 2, 2016

xiang90 commented Dec 2, 2016

krestenkrab Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

krestenkrab Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

krestenkrab Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

StupidHod commented Mar 17, 2017

StupidHod commented Sep 5, 2016 •

edited

Loading

oza Sep 6, 2016 •

edited

Loading

krestenkrab Mar 15, 2017 •

edited

Loading

krestenkrab Mar 15, 2017 •

edited

Loading

krestenkrab Mar 15, 2017 •

edited

Loading