api:support nested txn #265
Conversation
|
There is a failure on travis-ci, could you pease have a look ? |
|
it seems like tests failed. testNestedTxn(com.coreos.jetcd.internal.impl.KVTest) Time elapsed: 0.115 s <<< FAILURE!
java.util.concurrent.ExecutionException: java.util.concurrent.ExecutionException: io.grpc.StatusRuntimeException: INVALID_ARGUMENT: etcdserver: key not found
at com.coreos.jetcd.internal.impl.KVTest.testNestedTxn(KVTest.java:226)
Caused by: java.util.concurrent.ExecutionException: io.grpc.StatusRuntimeException: INVALID_ARGUMENT: etcdserver: key not found
Caused by: io.grpc.StatusRuntimeException: INVALID_ARGUMENT: etcdserver: key not foundalso the ci seems to be crazy i saw too many logs on downloading dependencies. |
|
you should run docker with https://github.com/coreos/jetcd/blob/master/etc/scripts/run_etcd_docker.sh then run you test against it. |
|
Actually I already tried this locally and then looked at the referenced issue at etcd (etcd-io/etcd#7857) there the API is destined for version 3.3. The docker configuration in the script uses version 3.2. Is there already a docker image of etcd with API version 3.3? |
|
@danielrohe i see what's going on. the nested txn is going to be in etcd 3.3 release. so we need to test the code again etcd 3.3. I believe there etcd v3.3.0 release candidate will be out this week. then you can test the code against rc. |
| @@ -15,7 +15,7 @@ | |||
| # limitations under the License. | |||
| # | |||
|
|
|||
| ETCD_VERSION="v3.2" | |||
| ETCD_VERSION="v3.3" | |||
There was a problem hiding this comment.
hmm, only v3.3.0-rc.0 is released https://github.com/coreos/etcd/releases/tag/v3.3.0-rc.0. does this script work locally?
Also i think the pr should be merged until the official v3.3.0 being released.
There was a problem hiding this comment.
The docker image at https://console.cloud.google.com/gcr/images/etcd-development/GLOBAL/etcd is tagged with v3.3 and v3.3.0-rc.0. This works locally and on travis-ci (https://travis-ci.org/coreos/jetcd/builds/323000589?utm_source=github_status&utm_medium=notification).
There was a problem hiding this comment.
@danielrohe have you taken a look on how nested txn is written in go client via this pr https://github.com/coreos/etcd/pull/8102/files?
I am currently reviewing this pr against that one. fyi.
There was a problem hiding this comment.
Yes, looked at this PR to see how it was implemented in Go and then converted to Java. Tried to follow your conventions with static methods and builders. Converted also the test case from Go to Java to see whether it really works.
|
please also modify TxnResponse.java to include the following: /**
* @returns a list of TxnResponse; empty list if none.
*/
public synchronized List<TxnResponse> getTxnResponses() {
if (txnResponses == null) {
txnResponses = getResponse().getResponsesList().stream()
.filter((responseOp) -> responseOp.getResponseCase() == RESPONSE_TXN)
.map(responseOp -> new TxnResponse(responseOp.getResponseTxn()))
.collect(Collectors.toList());
}
return txnResponses;
}Also please add a test in TxnResponseTest to verify that |
| @@ -194,4 +194,43 @@ public void testTxn() throws Exception { | |||
| test.assertEquals(getResp.getKvs().size(), 1); | |||
| test.assertEquals(getResp.getKvs().get(0).getValue().toStringUtf8(), putValue.toStringUtf8()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testNestedTxn() throws Exception { | |||
There was a problem hiding this comment.
Let's follow the format TestTxnNested from go client https://github.com/coreos/etcd/blob/master/clientv3/integration/txn_test.go#L193
@Test
public void testNestedTxn() throws Exception {
ByteSequence foo = ByteSequence.fromString("txn_foo");
ByteSequence bar = ByteSequence.fromString("txn_bar");
ByteSequence barz = ByteSequence.fromString("txn_barz");
ByteSequence abc = ByteSequence.fromString("txn_abc");
ByteSequence oneTwoThree = ByteSequence.fromString("txn_123");
Txn txn = kvClient.txn();
Cmp cmp = new Cmp(foo, Cmp.Op.EQUAL, CmpTarget.version(0));
CompletableFuture<com.coreos.jetcd.kv.TxnResponse> txnResp = txn.If(cmp)
.Then(Op.put(foo, bar, PutOption.DEFAULT),
Op.txn(null,
new Op[] {Op.put(abc, oneTwoThree, PutOption.DEFAULT)},
null))
.Else(Op.put(foo, barz, PutOption.DEFAULT)).commit();
txnResp.get();
GetResponse getResp = kvClient.get(foo).get();
test.assertEquals(getResp.getKvs().size(), 1);
test.assertEquals(getResp.getKvs().get(0).getValue().toStringUtf8(), bar.toStringUtf8());
GetResponse getResp2 = kvClient.get(abc).get();
test.assertEquals(getResp2.getKvs().size(), 1);
test.assertEquals(getResp2.getKvs().get(0).getValue().toStringUtf8(), oneTwoThree.toStringUtf8());
}
|
@danielrohe sorry for the delay. I have reviewed the code and made some suggestions. |
|
Adjusted code based on your suggestions. |
| assertThat(txnResponse.getTxnResponses().size()).isEqualTo(1); | ||
| } | ||
|
|
||
| } |
|
lgtm |
|
@danielrohe Please squash the commits into one, and fix the new line issue. We will get this merged once they are done. |
|
lgtm. can you squash the commits into one? or we can use github feature to get this squashed. |
|
already squashed them |
solves #143, how can it be tested locally with docker?