[to #439] introduce MockThreeStoresTest to onStoreUnreachable #518

iosmanthus · 2022-02-14T10:08:38Z

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

What problem does this PR solve?

Issue Number: close #439

This pull request tries to mock a TiKV grpc server and cover the error handling logic of client-java, mostly, the logic of seekLeaderStore and seekProxyStore in handleRequestError.

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

…eader-store

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

zz-jason · 2022-02-16T07:23:29Z

src/main/java/org/tikv/common/TiSession.java

@@ -166,6 +178,10 @@ private static VersionInfo getVersionInfo() {
  }

  private synchronized void warmUp() {
+    if (conf.isTest()) {
+      logger.info("skip warm up in test mode");


why skip warmup in test mode?

PDMockServerTest is the base class of MockServerTest.

TiSession is initialized in PDMockServerTest, and it calls warmup.

warmup calls getAllStores and rawGet.

This results in a dependent hell. We should ignore this anti-pattern warmup operation in TiSession, otherwise, we could not launch MockServerTest propperly.

Ignored warmup by setEnableWarmup(false)

zz-jason · 2022-02-16T07:25:56Z

src/test/java/org/tikv/common/KVMockServer.java

@@ -97,7 +124,7 @@ private void verifyContext(Context context) throws Exception {
    if (context.getRegionId() != region.getId()
        || !context.getRegionEpoch().equals(region.getRegionEpoch())
        || !context.getPeer().equals(region.getLeader())) {
-      throw new Exception();
+      throw new Exception("context doesn't match");


it would be better to specifically point out the mismatched field in context.

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

src/test/java/org/tikv/common/MockThreeStoresTest.java

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

marsishandsome · 2022-02-17T06:12:52Z

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

@@ -40,6 +40,7 @@
 import org.tikv.kvproto.Metapb.Region;

 public class TiRegion implements Serializable {
+


you can do code format in another PR

src/test/java/org/tikv/common/KVMockServer.java

marsishandsome · 2022-02-17T06:21:37Z

src/test/java/org/tikv/common/MockThreeStoresTest.java

+import org.tikv.kvproto.Metapb;
+import org.tikv.kvproto.Pdpb;
+
+public class MockThreeStoresTest extends PDMockServerTest {


rename MockThreeStoresTest to MockTiKVClusterTest?
The store number can be config?

I just want to keep it simple enough, for now, refactoring will be introduced in the future.

Create an issue to describe this refactoring?

Could I just add some comments to this class?

…eader-store

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

…-java into test-seek-leader-store

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

codecov · 2022-02-18T08:49:56Z

Codecov Report

Merging #518 (fe21429) into master (2ac49bb) will increase coverage by 0.95%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #518      +/-   ##
============================================
+ Coverage     30.94%   31.90%   +0.95%     
- Complexity     1282     1311      +29     
============================================
  Files           278      278              
  Lines         17343    17344       +1     
  Branches       1975     1975              
============================================
+ Hits           5367     5533     +166     
+ Misses        11387    11192     -195     
- Partials        589      619      +30

Impacted Files	Coverage Δ
src/main/java/org/tikv/common/TiSession.java	`71.15% <ø> (+0.48%)`	⬆️
src/main/java/org/tikv/common/region/TiRegion.java	`60.34% <ø> (+9.48%)`	⬆️
.../tikv/common/region/AbstractRegionStoreClient.java	`78.04% <100.00%> (+62.36%)`	⬆️
.../java/org/tikv/common/exception/GrpcException.java	`33.33% <0.00%> (-33.34%)`	⬇️
.../java/org/tikv/common/exception/TiKVException.java	`66.66% <0.00%> (-33.34%)`	⬇️
...ty/handler/codec/http2/Http2ConnectionHandler.java	`51.10% <0.00%> (-3.92%)`	⬇️
.../main/java/org/tikv/common/policy/RetryPolicy.java	`85.18% <0.00%> (-3.71%)`	⬇️
.../codec/http2/DefaultHttp2RemoteFlowController.java	`59.31% <0.00%> (-0.39%)`	⬇️
src/main/java/org/tikv/common/PDClient.java	`59.56% <0.00%> (ø)`
src/main/java/org/tikv/common/TiConfiguration.java	`65.98% <0.00%> (+0.20%)`	⬆️
... and 9 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 2ac49bb...fe21429. Read the comment docs.

marsishandsome

LGTM

zz-jason

LGTM

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

…eader-store

…-java into test-seek-leader-store

wip: introduce MockThreeStoresTest to test seek leader store

035befc

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

github-actions bot added the component/common label Feb 14, 2022

iosmanthus added 2 commits February 15, 2022 22:47

finish first stage test for seekLeaderStore

a8a4fa2

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

./dev/javafmt and fix failed test

e485695

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

iosmanthus changed the title ~~[to #439] [WIP] introduce MockThreeStoresTest to test seek leader store~~ [to #439] introduce MockThreeStoresTest to test seek leader store Feb 16, 2022

add license header in MockThreeStoresTest

0aff000

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

iosmanthus requested review from marsishandsome, birdstorm and zz-jason February 16, 2022 06:59

iosmanthus added 2 commits February 16, 2022 15:04

Merge branch 'master' of github.com:tikv/client-java into test-seek-l…

1e0736a

…eader-store

remove ignored exception log

fa7ca88

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

zz-jason reviewed Feb 16, 2022

View reviewed changes

./dev/javafmt

a374f01

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

marsishandsome reviewed Feb 17, 2022

View reviewed changes

iosmanthus mentioned this pull request Feb 17, 2022

Mock server plan #531

Open

7 tasks

Merge branch 'master' of github.com:tikv/client-java into test-seek-l…

a5438fa

…eader-store

iosmanthus linked an issue Feb 17, 2022 that may be closed by this pull request

Mock server plan #531

Open

7 tasks

iosmanthus added 2 commits February 17, 2022 19:36

add unit tests for seekProxyStore

838a1b7

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

add comments to SeekProxyStoreTest

05e1089

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

iosmanthus changed the title ~~[to #439] introduce MockThreeStoresTest to test seek leader store~~ [to #439] introduce MockThreeStoresTest to onStoreUnreachable Feb 17, 2022

iosmanthus and others added 6 commits February 17, 2022 19:41

fix license error

1175bde

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

fix license error

81058eb

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

Merge branch 'master' into test-seek-leader-store

3f2a55b

enable grpc forward in PDMockServerTest

ffeaa1f

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

Merge branch 'test-seek-leader-store' of github.com:iosmanthus/client…

cdc9d57

…-java into test-seek-leader-store

remove ignore warmup for tests

152e3d2

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

marsishandsome approved these changes Feb 21, 2022

View reviewed changes

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

Merge branch 'master' into test-seek-leader-store

5cee439

zz-jason approved these changes Feb 21, 2022

View reviewed changes

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

ti-srebot added status/LGT2 and removed status/LGT1 labels Feb 21, 2022

ti-srebot approved these changes Feb 21, 2022

View reviewed changes

iosmanthus added 2 commits February 21, 2022 15:40

Merge branch 'master' of github.com:tikv/client-java into test-seek-l…

569f925

…eader-store

Merge branch 'test-seek-leader-store' of github.com:iosmanthus/client…

fe21429

…-java into test-seek-leader-store

zz-jason merged commit d24b8e9 into tikv:master Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[to #439] introduce MockThreeStoresTest to onStoreUnreachable #518

[to #439] introduce MockThreeStoresTest to onStoreUnreachable #518

iosmanthus commented Feb 14, 2022 •

edited

zz-jason Feb 16, 2022

iosmanthus Feb 16, 2022

iosmanthus Feb 18, 2022

zz-jason Feb 16, 2022

marsishandsome Feb 17, 2022

marsishandsome Feb 17, 2022

iosmanthus Feb 17, 2022 •

edited

marsishandsome Feb 18, 2022

iosmanthus Feb 18, 2022

marsishandsome Feb 21, 2022

codecov bot commented Feb 18, 2022 •

edited

marsishandsome left a comment

zz-jason left a comment

		@@ -40,6 +40,7 @@
		import org.tikv.kvproto.Metapb.Region;

		public class TiRegion implements Serializable {

[to #439] introduce MockThreeStoresTest to onStoreUnreachable #518

[to #439] introduce MockThreeStoresTest to onStoreUnreachable #518

Conversation

iosmanthus commented Feb 14, 2022 • edited

What problem does this PR solve?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iosmanthus Feb 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2022 • edited

Codecov Report

marsishandsome left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

iosmanthus commented Feb 14, 2022 •

edited

iosmanthus Feb 17, 2022 •

edited

codecov bot commented Feb 18, 2022 •

edited