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

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

Signed-off-by: iosmanthus myosmanthustree@gmail.com

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 <myosmanthustree@gmail.com>

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 <myosmanthustree@gmail.com>

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 <myosmanthustree@gmail.com>

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

    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 <myosmanthustree@gmail.com>

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 <ti-srebot@pingcap.com>

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

cherry pick to release-3.1 in PR #545

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

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

cherry pick to release-3.2 in PR #546

Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: iosmanthus <dengliming@pingcap.com>

This reverts commit ad6a00f.

…541) (#548) * rawkv: cherry-pick fix-scan-empty-key Signed-off-by: iosmanthus <myosmanthustree@gmail.com> * rawkv: fix missing parameters Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: iosmanthus <dengliming@pingcap.com>

iosmanthus added 2 commits February 28, 2022 15:05

rawkv: fix scan return empty set while exist empty key

0414196

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

rawkv: ./dev/javafmt

4857521

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

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

iosmanthus requested review from birdstorm, marsishandsome 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 <myosmanthustree@gmail.com>

rawkv: rename eof to endOfScan

a38972e

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

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 <myosmanthustree@gmail.com>

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 <ti-srebot@pingcap.com>

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 <ti-srebot@pingcap.com>

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 <ti-srebot@pingcap.com> Co-authored-by: iosmanthus <dengliming@pingcap.com>

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 <ti-srebot@pingcap.com> Co-authored-by: iosmanthus <dengliming@pingcap.com>

[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

Uh oh!

Uh oh!

zz-jason commented Feb 28, 2022

Uh oh!

iosmanthus commented Feb 28, 2022

Uh oh!

codecov bot commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marsishandsome Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

iosmanthus Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

marsishandsome Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

marsishandsome left a comment

Choose a reason for hiding this comment

Uh oh!

marsishandsome Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zz-jason left a comment

Choose a reason for hiding this comment

Uh oh!

iosmanthus commented Mar 1, 2022

Uh oh!

ti-srebot commented Mar 1, 2022

Uh oh!

zz-jason commented Mar 1, 2022

Uh oh!

iosmanthus commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-srebot commented Mar 1, 2022

Uh oh!

ti-srebot commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 28, 2022 •

edited

Loading

iosmanthus commented Mar 1, 2022 •

edited

Loading