Add CompareAndSet for RawClient and make Get returns Optional by andylokandy · Pull Request #192 · tikv/client-java

andylokandy · 2021-06-11T08:36:10Z

What is changed and how it works?

Removed putIfAbsent
Added CompareAndSet
Make Get returns Optional

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change

Related changes

Need to update the documentation
Need to be included in the release note

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

src/main/java/org/tikv/raw/RawKVClient.java

src/main/java/org/tikv/common/exception/RawCASConflictException.java

src/test/java/org/tikv/raw/RawKVClientTest.java

marsishandsome

@birdstorm @Little-Wallace this PR changes the public API. I'm wondering whether the compatibility problem will affect users?

iosmanthus · 2021-06-11T09:31:21Z

@andylokandy I don't think removing putIfAbsent is a good idea. What if the semantic of "key does not exist" changes?

src/main/java/org/tikv/common/region/RegionStoreClient.java

Signed-off-by: Andy Lok <andylokandy@hotmail.com> Co-authored-by: Liangliang Gu <marsishandsome@gmail.com>

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

marsishandsome · 2021-06-15T02:45:15Z

/run-all-tests

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

andylokandy · 2021-06-15T07:59:37Z

@birdstorm @marsishandsome PTAL

marsishandsome

other LGTM

src/main/java/org/tikv/raw/RawKVClient.java

.settings/org.eclipse.core.resources.prefs

src/main/java/org/tikv/raw/RawKVClient.java

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

marsishandsome

LGTM

marsishandsome · 2021-06-17T02:22:07Z

@birdstorm PTAL

birdstorm

LGTM

marsishandsome · 2021-06-17T02:25:56Z

/merge

ti-srebot · 2021-06-17T02:28:20Z

/run-all-tests

ti-srebot · 2021-06-17T02:32:20Z

@andylokandy merge failed.

marsishandsome · 2021-06-17T02:34:30Z

@andylokandy test failed

[2021-06-17T02:31:49.545Z] [ERROR] /home/jenkins/agent/git/client-java/src/test/java/org/tikv/raw/RawKVClientTest.java:[123,41] incompatible types: java.util.Optional<com.google.protobuf.ByteString> cannot be converted to com.google.protobuf.ByteString

[2021-06-17T02:31:49.545Z] [ERROR] /home/jenkins/agent/git/client-java/src/test/java/org/tikv/raw/RawKVClientTest.java:[125,41] incompatible types: java.util.Optional<com.google.protobuf.ByteString> cannot be converted to com.google.protobuf.ByteString

[2021-06-17T02:31:49.545Z] [ERROR] /home/jenkins/agent/git/client-java/src/test/java/org/tikv/raw/RawKVClientTest.java:[132,41] incompatible types: java.util.Optional<com.google.protobuf.ByteString> cannot be converted to com.google.protobuf.ByteString

marsishandsome · 2021-06-17T06:06:16Z

@andylokandy why use the funciton name compareAndSet instead of compareAndSwap like the rust client does?

src/main/java/org/tikv/common/exception/RawCASConflictException.java

src/main/java/org/tikv/raw/RawKVClient.java

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

tisonkun · 2021-06-17T07:36:08Z

/run-all-tests

tisonkun

Thanks for your updates @andylokandy ! I left an inline comment on testing part. PTAL.

src/test/java/org/tikv/raw/RawKVClientTest.java

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

andylokandy · 2021-06-17T18:09:29Z

@birdstorm @tisonkun @marsishandsome PTAL

tisonkun · 2021-06-18T01:14:03Z

/run-all-tests

tisonkun

Generally LGTM. Thanks for your effort!

You may miss the comment https://github.com/tikv/client-java/pull/192/files#r653291717 .

ti-srebot · 2021-06-18T01:20:53Z

@tisonkun, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. You are not a reviewer or committer or co-leader or leader.

tisonkun · 2021-06-18T01:24:42Z

https://ci.pingcap.net/blue/organizations/jenkins/tikv-client-java_ghpr_integration_test/detail/tikv-client-java_ghpr_integration_test/41/pipeline

@andylokandy @marsishandsome test failed. Is it due to the test infra settings not upgrade?

marsishandsome · 2021-06-18T02:21:27Z

https://ci.pingcap.net/blue/organizations/jenkins/tikv-client-java_ghpr_integration_test/detail/tikv-client-java_ghpr_integration_test/41/pipeline

@andylokandy @marsishandsome test failed. Is it due to the test infra settings not upgrade?

cas is supported in v5.0, but current test uses v4.0.
please rebase this pr #202

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

Add CompareAndSet for RawClient and make Get returns Optional

cf18ce8

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

andylokandy requested review from Little-Wallace, birdstorm and marsishandsome June 11, 2021 08:36

marsishandsome reviewed Jun 11, 2021

View reviewed changes

src/main/java/org/tikv/raw/RawKVClient.java Outdated Show resolved Hide resolved

src/main/java/org/tikv/common/exception/RawCASConflictException.java Outdated Show resolved Hide resolved

src/test/java/org/tikv/raw/RawKVClientTest.java Outdated Show resolved Hide resolved

marsishandsome reviewed Jun 11, 2021

View reviewed changes

src/main/java/org/tikv/common/region/RegionStoreClient.java Show resolved Hide resolved

andylokandy and others added 2 commits June 11, 2021 18:17

Apply suggestions from code review

203ce51

Signed-off-by: Andy Lok <andylokandy@hotmail.com> Co-authored-by: Liangliang Gu <marsishandsome@gmail.com>

Format code

9c14257

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

andylokandy added 2 commits June 15, 2021 13:27

Add putIfAbsent

7a102b6

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

Rename sth

438bdf9

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

marsishandsome reviewed Jun 15, 2021

View reviewed changes

src/main/java/org/tikv/raw/RawKVClient.java Show resolved Hide resolved

birdstorm reviewed Jun 15, 2021

View reviewed changes

.settings/org.eclipse.core.resources.prefs Outdated Show resolved Hide resolved

src/main/java/org/tikv/raw/RawKVClient.java Show resolved Hide resolved

andylokandy added 3 commits June 16, 2021 14:41

Remove .vscode

dc60a08

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

Remove .settings

2d892d9

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

Delete .classpath

cdb103e

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

marsishandsome approved these changes Jun 16, 2021

View reviewed changes

ti-srebot added the status/LGT1 label Jun 16, 2021

birdstorm approved these changes Jun 17, 2021

View reviewed changes

ti-srebot removed the status/LGT1 label Jun 17, 2021

ti-srebot approved these changes Jun 17, 2021

View reviewed changes

ti-srebot added the status/LGT2 label Jun 17, 2021

ti-srebot added the status/can-merge label Jun 17, 2021

Merge branch 'master' into master

8ea8c45

marsishandsome reviewed Jun 17, 2021

View reviewed changes

src/main/java/org/tikv/common/exception/RawCASConflictException.java Show resolved Hide resolved

marsishandsome reviewed Jun 17, 2021

View reviewed changes

src/main/java/org/tikv/raw/RawKVClient.java Show resolved Hide resolved

Fix deadloop

4e79ed7

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

tisonkun reviewed Jun 17, 2021

View reviewed changes

src/test/java/org/tikv/raw/RawKVClientTest.java Show resolved Hide resolved

marsishandsome reviewed Jun 17, 2021

View reviewed changes

src/test/java/org/tikv/raw/RawKVClientTest.java Outdated Show resolved Hide resolved

andylokandy added 3 commits June 17, 2021 17:29

Enable TTL and CAS test

80ca8bd

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

Merge remote-tracking branch 'origin/master'

49ae106

Fix test

d4eac7f

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

andylokandy force-pushed the master branch from deec92a to d4eac7f Compare June 17, 2021 10:57

tisonkun approved these changes Jun 18, 2021

View reviewed changes

marsishandsome mentioned this pull request Jun 18, 2021

update test tidb version to release-5.0 and enable ttl #202

Closed

rebase tikv#202

5fdd79c

Signed-off-by: Andy Lok <andylokandy@hotmail.com>

andylokandy merged commit 8308d79 into tikv:master Jun 18, 2021

Conversation

andylokandy commented Jun 11, 2021

What is changed and how it works?

Check List

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marsishandsome left a comment

Choose a reason for hiding this comment

Uh oh!

iosmanthus commented Jun 11, 2021

Uh oh!

Uh oh!

marsishandsome commented Jun 15, 2021

Uh oh!

andylokandy commented Jun 15, 2021

Uh oh!

marsishandsome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marsishandsome left a comment

Choose a reason for hiding this comment

Uh oh!

marsishandsome commented Jun 17, 2021

Uh oh!

birdstorm left a comment

Choose a reason for hiding this comment

Uh oh!

marsishandsome commented Jun 17, 2021

Uh oh!

ti-srebot commented Jun 17, 2021

Uh oh!

ti-srebot commented Jun 17, 2021

Uh oh!

marsishandsome commented Jun 17, 2021

Uh oh!

marsishandsome commented Jun 17, 2021

Uh oh!

Uh oh!

Uh oh!

tisonkun commented Jun 17, 2021

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andylokandy commented Jun 17, 2021

Uh oh!

tisonkun commented Jun 18, 2021

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

ti-srebot commented Jun 18, 2021

Uh oh!

tisonkun commented Jun 18, 2021

Uh oh!

marsishandsome commented Jun 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants