Convert Java client to asynchronous RPCs.

[enisoc]

@erzel

To get the old synchronous behavior, append .checkedGet() to a call
that returns SQLFuture. For example:

Cursor cursor = vtgateConn.execute(...).checkedGet();

[erzel]

Reviewed 38 of 38 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

java/client/src/main/java/com/youtube/vitess/client/SQLFuture.java, line 27 [r1] (raw file):
It's a little confusing here to say that you don't implement that interface because you do, you just don't declare that you do.
So perhaps rephrase to say: "this class does not declare that it implements the interface"

---

java/client/src/main/java/com/youtube/vitess/client/SQLFuture.java, line 97 [r1] (raw file):
So you're wrapping the ExecutionException in the SQLException not the underlying SQLException?

---

java/client/src/main/java/com/youtube/vitess/client/VTGateTx.java, line 42 [r1] (raw file):
Agreed. I'm wondering what are the use-cases for an asynchronous Transaction API. It would simplify this code if we can get away with only allowing a synchronous API.
If we do allow asynchronous calls here I think you should add enforcement for this (e.g. throw an exception if there's already an op in progress). Currently, if the caller issues two queries after a begin() they could end up executed in different MySQL connections.

---

java/client/src/main/java/com/youtube/vitess/client/VTGateTx.java, line 43 [r1] (raw file):
I'm unclear as to why you need synchronized in the Vitess API methods.

---

java/grpc-client/src/main/java/com/youtube/vitess/client/grpc/GrpcStreamAdapter.java, line 48 [r1] (raw file):
Did GRPC change their API?

---

Comments from the review on Reviewable.io

[erzel]

Looks nice. Sorry for the delay in reviewing.

---

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from the review on Reviewable.io

[enisoc]

PTAL

---

Review status: 36 of 38 files reviewed at latest revision, 5 unresolved discussions.

---

java/client/src/main/java/com/youtube/vitess/client/SQLFuture.java, line 27 [r1] (raw file):
Done.

---

java/client/src/main/java/com/youtube/vitess/client/SQLFuture.java, line 97 [r1] (raw file):
Yes, although I was torn about whether that's the right thing to do. In the end, I thought more information is better?

---

java/client/src/main/java/com/youtube/vitess/client/VTGateTx.java, line 42 [r1] (raw file):
Certainly for commit() I could imagine wanting to do other work while waiting for the server to get back to you. You can also do reads as part of a transaction, and you may want those async too for the same reasons our customers want async queries in general. Since the underlying RpcClient interface is async anyway, the alternative is to have VTGateTx call checkedGet() for you. I'd rather leave the option in the user's hands.

I agree it would be nice to enforce the in-flight operation limit. Originally I was thinking the second attempted operation would block until the previous one finished, but that seemed too complicated and prone to deadlock. I like your suggestion to just throw an exception to say that's not allowed.

---

java/client/src/main/java/com/youtube/vitess/client/VTGateTx.java, line 43 [r1] (raw file):
The session field gets updated by the async callback, which could be in a different thread. In order for the next call to execute (for example) to be guaranteed to see the new session value, it must be synchronized. Otherwise the Java memory model allows it to see the old value from a thread-local cache.

---

java/grpc-client/src/main/java/com/youtube/vitess/client/grpc/GrpcStreamAdapter.java, line 48 [r1] (raw file):
Yes I updated the Maven dependency to grpc-java 0.12 to match the rest of our project being updated to grpc 0.12.

---

Comments from the review on Reviewable.io

[erzel]

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

java/client/src/main/java/com/youtube/vitess/client/SQLFuture.java, line 97 [r1] (raw file):
SGTM

---

java/client/src/main/java/com/youtube/vitess/client/VTGateTx.java, line 42 [r1] (raw file):
Fair enough. I think you've convinced me about the need for asynchronicity in transactions.

---

java/client/src/main/java/com/youtube/vitess/client/VTGateTx.java, line 43 [r1] (raw file):
You're right. I was confused about the meaning of synchronized in Java.
Can you just move the word "only" to after the word "synchronized" in the comment to make this slightly clearer.

---

java/grpc-client/src/main/java/com/youtube/vitess/client/grpc/GrpcStreamAdapter.java, line 48 [r1] (raw file):
LGTM, but please see one last comment.

---

Comments from the review on Reviewable.io

[enisoc]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

java/client/src/main/java/com/youtube/vitess/client/VTGateTx.java, line 43 [r1] (raw file):
Done.

---

Comments from the review on Reviewable.io