[close #540] rawkv: fix scan return empty set while exist empty key #541

iosmanthus · 2022-02-28T07:09:03Z

Signed-off-by: iosmanthus [email protected]

What problem does this PR solve?

Issue Number: close #540

Problem Description:

This pull request adds an empty key check for RawScanIterator.hasNext, otherwise, if there is an empty key in TiKV, the scan will always return an empty set.

Check List for Tests

This PR has been tested by at least one of the following methods:

Integration test

Signed-off-by: iosmanthus <[email protected]>

src/main/java/org/tikv/common/operation/iterator/RawScanIterator.java

zz-jason · 2022-02-28T07:25:30Z

test failed, please take a look:

[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    RawKVClientTest.scanTestForIssue540:385 expected:<1> but was:<2>
[INFO] 
Error:  Tests run: 77, Failures: 1, Errors: 0, Skipped: 0

Signed-off-by: iosmanthus <[email protected]>

iosmanthus · 2022-02-28T07:44:53Z

test failed, please take a look:

[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    RawKVClientTest.scanTestForIssue540:385 expected:<1> but was:<2>
[INFO] 
Error:  Tests run: 77, Failures: 1, Errors: 0, Skipped: 0

Fixed, PTAL

Signed-off-by: iosmanthus <[email protected]>

codecov · 2022-02-28T07:52:05Z

Codecov Report

Merging #541 (69ab375) into master (36feccb) will increase coverage by 0.14%.
The diff coverage is 80.00%.

@@             Coverage Diff              @@
##             master     #541      +/-   ##
============================================
+ Coverage     31.72%   31.87%   +0.14%     
- Complexity     1303     1320      +17     
============================================
  Files           278      278              
  Lines         17344    17345       +1     
  Branches       1975     1975              
============================================
+ Hits           5503     5528      +25     
+ Misses        11223    11215       -8     
+ Partials        618      602      -16

Impacted Files	Coverage Δ
...g/tikv/common/operation/iterator/ScanIterator.java	`73.68% <ø> (+13.15%)`	⬆️
...ikv/common/operation/iterator/RawScanIterator.java	`82.75% <80.00%> (+14.90%)`	⬆️
src/main/java/io/grpc/netty/WriteQueue.java	`74.43% <0.00%> (-2.26%)`	⬇️
...ty/handler/codec/http2/Http2ConnectionHandler.java	`46.94% <0.00%> (-1.23%)`	⬇️
...rc/main/java/io/grpc/netty/NettyClientHandler.java	`55.81% <0.00%> (-0.87%)`	⬇️
src/main/java/org/tikv/common/PDClient.java	`59.07% <0.00%> (-0.49%)`	⬇️
src/main/java/org/tikv/common/TiSession.java	`70.67% <0.00%> (-0.49%)`	⬇️
src/main/java/io/grpc/internal/ClientCallImpl.java	`57.07% <0.00%> (+1.21%)`	⬆️
src/main/java/org/tikv/raw/RawKVClient.java	`61.65% <0.00%> (+1.45%)`	⬆️
src/main/java/io/grpc/stub/ClientCalls.java	`48.51% <0.00%> (+2.31%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36feccb...69ab375. Read the comment docs.

marsishandsome · 2022-02-28T08:05:26Z

src/main/java/org/tikv/common/operation/iterator/RawScanIterator.java

+    if (!processingLastBatch) {
+      return false;
+    }


if limit > 0 && processingLastBatch = false:

your code returns false

origional code returns true

In hasNext, it call !endOfScan() instead of notEndOfScan which is more confusing.

ok, I understand.
I recommend to separate bug fix and code refactor in 2 PRs.

marsishandsome

others, LGTM

marsishandsome · 2022-02-28T09:04:54Z

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

@@ -360,6 +378,16 @@ public void simpleTest() {
    return client.scan(RAW_START_KEY, RAW_END_KEY);
  }

+  // https://github.com/tikv/client-java/issues/540


we can find the issue link according to the commit id.

src/main/java/org/tikv/common/operation/iterator/RawScanIterator.java

Signed-off-by: iosmanthus <[email protected]>

zz-jason

LGTM

iosmanthus · 2022-03-01T03:17:56Z

/merge

ti-srebot · 2022-03-01T03:18:06Z

/run-all-tests

zz-jason · 2022-03-01T03:28:44Z

@iosmanthus test failed:

Error:  Errors: 
Error:    RawKVIngestTest.rawKVIngestTestWithTTL:113 » Grpc io.grpc.StatusRuntimeExcepti...
Error:    CASTest.rawPutIfAbsentTest:89 » Grpc retry is exhausted.
Error:    RawKVClientTest.getKeyTTLTest:142 » Grpc retry is exhausted.
[INFO] 
Error:  Tests run: 77, Failures: 0, Errors: 3, Skipped: 0

iosmanthus · 2022-03-01T05:52:57Z

@iosmanthus test failed:

Error:  Errors: 
Error:    RawKVIngestTest.rawKVIngestTestWithTTL:113 » Grpc io.grpc.StatusRuntimeExcepti...
Error:    CASTest.rawPutIfAbsentTest:89 » Grpc retry is exhausted.
Error:    RawKVClientTest.getKeyTTLTest:142 » Grpc retry is exhausted.
[INFO] 
Error:  Tests run: 77, Failures: 0, Errors: 3, Skipped: 0

It should be an upstream issue.

2022-02-28T14:02:09.8501660Z io.grpc.StatusRuntimeException: UNKNOWN: TTLNotEnabled
2022-02-28T14:02:09.8502057Z 	at io.grpc.Status.asRuntimeException(Status.java:535)
2022-02-28T14:02:09.8502573Z 	at io.grpc.stub.ClientCalls$StreamObserverToCallListenerAdapter.onClose(ClientCalls.java:506)
2022-02-28T14:02:09.8503138Z 	at io.grpc.internal.ClientCallImpl.closeObserver(ClientCallImpl.java:577)
2022-02-28T14:02:09.8503797Z 	at io.grpc.internal.ClientCallImpl.access$300(ClientCallImpl.java:68)
2022-02-28T14:02:09.8504371Z 	at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1StreamClosed.runInternal(ClientCallImpl.java:786)
2022-02-28T14:02:09.8505069Z 	at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1StreamClosed.runInContext(ClientCallImpl.java:764)
2022-02-28T14:02:09.8505618Z 	at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37)
2022-02-28T14:02:09.8506101Z 	at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:123)
2022-02-28T14:02:09.8506658Z 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
2022-02-28T14:02:09.8507232Z 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
2022-02-28T14:02:09.8507651Z 	at java.lang.Thread.run(Thread.java:748)
2022-02-28T14:02:09.8508408Z [ERROR] Tests run: 2, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 218.773 s <<< FAILURE! - in org.tikv.common.importer.RawKVIngestTest
2022-02-28T14:02:09.8509060Z [ERROR] org.tikv.common.importer.RawKVIngestTest.rawKVIngestTestWithTTL  Time elapsed: 123.311 s  <<< ERROR!
2022-02-28T14:02:09.8509707Z org.tikv.common.exception.GrpcException: io.grpc.StatusRuntimeException: UNKNOWN: TTLNotEnabled
2022-02-28T14:02:09.8510348Z 	at org.tikv.common.importer.RawKVIngestTest.rawKVIngestTestWithTTL(RawKVIngestTest.java:113)
2022-02-28T14:02:09.8512194Z Caused by: io.grpc.StatusRuntimeException: UNKNOWN: TTLNotEnabled

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2022-03-01T06:36:21Z

cherry pick to release-3.1 in PR #545

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2022-03-01T06:37:09Z

cherry pick to release-3.2 in PR #546

Signed-off-by: ti-srebot <[email protected]> Co-authored-by: iosmanthus <[email protected]>

This reverts commit ad6a00f.

…541) (#548) * rawkv: cherry-pick fix-scan-empty-key Signed-off-by: iosmanthus <[email protected]> * rawkv: fix missing parameters Signed-off-by: iosmanthus <[email protected]>

Signed-off-by: ti-srebot <[email protected]> Co-authored-by: iosmanthus <[email protected]>

iosmanthus added 2 commits February 28, 2022 15:05

rawkv: fix scan return empty set while exist empty key

0414196

Signed-off-by: iosmanthus <[email protected]>

rawkv: ./dev/javafmt

4857521

Signed-off-by: iosmanthus <[email protected]>

iosmanthus added the needs-cherry-pick-3.1 label Feb 28, 2022

iosmanthus requested review from marsishandsome, birdstorm and zz-jason February 28, 2022 07:09

zz-jason reviewed Feb 28, 2022

View reviewed changes

src/main/java/org/tikv/common/operation/iterator/RawScanIterator.java Outdated Show resolved Hide resolved

rawkv: clean tikv data before scanTest

c4a4a0c

Signed-off-by: iosmanthus <[email protected]>

rawkv: rename eof to endOfScan

a38972e

Signed-off-by: iosmanthus <[email protected]>

marsishandsome reviewed Feb 28, 2022

View reviewed changes

marsishandsome previously approved these changes Feb 28, 2022

View reviewed changes

ti-srebot added the status/LGT1 label Feb 28, 2022

zz-jason reviewed Feb 28, 2022

View reviewed changes

src/main/java/org/tikv/common/operation/iterator/RawScanIterator.java Outdated Show resolved Hide resolved

rawkv: remove extra cond

a62f09e

Signed-off-by: iosmanthus <[email protected]>

iosmanthus dismissed marsishandsome’s stale review via a62f09e February 28, 2022 13:38

Merge branch 'master' into fix-scan-empty-key

69ab375

zz-jason added the needs-cherry-pick-3.2 label Feb 28, 2022

zz-jason approved these changes Mar 1, 2022

View reviewed changes

ti-srebot removed the status/LGT1 label Mar 1, 2022

ti-srebot approved these changes Mar 1, 2022

View reviewed changes

ti-srebot added the status/LGT2 label Mar 1, 2022

ti-srebot added the status/can-merge label Mar 1, 2022

marsishandsome merged commit 9798382 into tikv:master Mar 1, 2022

ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Mar 1, 2022

cherry pick tikv#541 to release-3.1

47e318a

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Mar 1, 2022

[close #540] rawkv: fix scan return empty set while exist empty key (#541) #545

Merged

ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Mar 1, 2022

cherry pick tikv#541 to release-3.2

68b18db

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Mar 1, 2022

[close #540] rawkv: fix scan return empty set while exist empty key (#541) #546

Merged

marsishandsome pushed a commit that referenced this pull request Mar 1, 2022

cherry pick #541 to release-3.1 (#545)

ad6a00f

Signed-off-by: ti-srebot <[email protected]> Co-authored-by: iosmanthus <[email protected]>

marsishandsome added a commit that referenced this pull request Mar 1, 2022

Revert "cherry pick #541 to release-3.1 (#545)"

6b03d82

This reverts commit ad6a00f.

marsishandsome added a commit that referenced this pull request Mar 1, 2022

Revert "cherry pick #541 to release-3.1 (#545)" (#547)

d7313e5

This reverts commit ad6a00f.

iosmanthus mentioned this pull request Mar 1, 2022

[close #540] rawkv: fix scan return empty set while exist empty key (#541) #548

Merged

zz-jason pushed a commit that referenced this pull request Mar 22, 2022

cherry pick #541 to release-3.2 (#546)

101c949

Signed-off-by: ti-srebot <[email protected]> Co-authored-by: iosmanthus <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[close #540] rawkv: fix scan return empty set while exist empty key #541

[close #540] rawkv: fix scan return empty set while exist empty key #541

iosmanthus commented Feb 28, 2022

zz-jason commented Feb 28, 2022

iosmanthus commented Feb 28, 2022

codecov bot commented Feb 28, 2022 •

edited

Loading

marsishandsome Feb 28, 2022

iosmanthus Feb 28, 2022

marsishandsome Feb 28, 2022

marsishandsome left a comment

marsishandsome Feb 28, 2022

zz-jason left a comment

iosmanthus commented Mar 1, 2022

ti-srebot commented Mar 1, 2022

zz-jason commented Mar 1, 2022

iosmanthus commented Mar 1, 2022 •

edited

Loading

ti-srebot commented Mar 1, 2022

ti-srebot commented Mar 1, 2022

[close #540] rawkv: fix scan return empty set while exist empty key #541

[close #540] rawkv: fix scan return empty set while exist empty key #541

Conversation

iosmanthus commented Feb 28, 2022

What problem does this PR solve?

Check List for Tests

zz-jason commented Feb 28, 2022

iosmanthus commented Feb 28, 2022

codecov bot commented Feb 28, 2022 • edited Loading

Codecov Report

marsishandsome Feb 28, 2022

Choose a reason for hiding this comment

iosmanthus Feb 28, 2022

Choose a reason for hiding this comment

marsishandsome Feb 28, 2022

Choose a reason for hiding this comment

marsishandsome left a comment

Choose a reason for hiding this comment

marsishandsome Feb 28, 2022

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

iosmanthus commented Mar 1, 2022

ti-srebot commented Mar 1, 2022

zz-jason commented Mar 1, 2022

iosmanthus commented Mar 1, 2022 • edited Loading

ti-srebot commented Mar 1, 2022

ti-srebot commented Mar 1, 2022

codecov bot commented Feb 28, 2022 •

edited

Loading

iosmanthus commented Mar 1, 2022 •

edited

Loading