FLASH-275/276/277/278/281/282: Complete DDL implementation#110

Merged

zanmato1984 merged 135 commits intomasterfrom

Aug 5, 2019

Contributor

zanmato1984 commented Jul 18, 2019 •

edited

Loading

Doc links: design doc, test doc

There are several relative individual parts of this huge PR:

Getting schema from TiKV instead of TiDB:
1.1 TiKV client fixes: contrib/client-c/*
1.2 Schema syncer: the old TiDB-based syncer is moved to Debug dir for mock usage only, ./ dbms/src/Storages/Transaction/SchemaSyncer.h only contains the interface, the new TiKV-based schema syncer is ./ dbms/src/Storages/Transaction/TiDBSchemaSyncer.h (the naming is terrible though) and related util classes (SchemaGetter and SchemaBuilder)
1.3 Table info json format change, therefore reimplemented in dbms/src/Storages/Transaction/TiDB.*
1.4 Remove all the stuff that concerns TiDB schema syncer: curl dependency, schema_sync test, TiDBService and related configs, table ignoring config being moved to TMTContext
Background schema syncing service is in dbms/src/Storages/Transaction/SchemaSyncService.* and related Context and Server changes.
Avoiding data writing by data widening for potential future column type change (only int widening is supported in TiDB) is in DataType* files and related calls.
Schema sync calling when reading/writing:
4.1 Refine flushRegion in RegionTable and underlying PartitionStream/RegionBlockReader by considering schema being out-of-date
4.2 Refine IntepreterSelectQuery by considering schema alignment between upper end (TiSpark/chspark/TiDB) and lower end (CH).
Testing:
5.1 Lots of debug function for schema syncing and mock TiDB table
5.2 Lots of tests using mock TiDB and schema syncer

zanmato1984 and others added 30 commits

June 11, 2019 15:07


          Widen schema syncer with sync all semantic, add background schema syn…

a514e64

…c service and do init sync on startup


          Merge branch 'master' into ddl

4a79bb7


          Finish widen some data types to avoid data rewriting during apply ddl

0eafdd4


          Merge branch 'master' into ddl

9d69885


          Add update_timestamp into table info

5b648cc


          Refine read and flush to be more robust for schema change

647bbbb


          Fix nullable for overflow check when decode region cache

bf25957


          Add schema state enum

095ca6c


          Support column type change in schema syncer

dd50f03


          Fix comment

363ecc7


          Add mock functions for TiDB DDL

18ed32e


          Fix rvalue ref

402132a


          Fix some bugs in schema syncer and mock tidb

02ad15e


          Adjust test

fb2b960


          Add alter test

718d1c0


          Not do data conversion for alter txn table, as we already stored wide…

463836e

…n data


          Fix typo

e86d96a


          Fix mock tidb reverse type mapping

6988bdb


          Enhance overflow checking when flushing

7fb48af


          Enhance alter test

9ad7a1c


          Refine some code

509ee1c


          Remove table region mapping when drop table

f26a75e


          Adjust drop order

261b3d2


          Print file path in mutable test script when error

47d17fc


          Fix insert widen data through sql

d409926


          Fix some txn tests

40379bf


          Fix widen data write of import statement

ebd897a


          DDL read and update by tikv's meta (#103)

98d09cf

* DDL read and update by tikv's meta

* add dbg back

* refine test

* address comments


          Add schema version check before reading

423363a


          Merge branch 'ddl' into ddl-ruoxi

c6f32a6

Contributor Author

zanmato1984 commented Jul 31, 2019

/run-integration-tests

innerr reviewed

View reviewed changes

contrib/client-c/include/tikv/Region.h Show resolved Hide resolved

contrib/client-c/include/tikv/Rpc.h Outdated

+                  }
+                  static ::grpc::Status doRPCCall(grpc::ClientContext * context, std::unique_ptr<tikvpb::Tikv::Stub> stub, const RequestType & req, ResultType * res) {
+                      return stub -> ReadIndex(context, req, res);

Contributor

innerr Aug 1, 2019

stub -> ReadIndex => stub->ReadIndex

contrib/client-c/include/tikv/Rpc.h Outdated

+                  using RequestType = kvrpcpb::ReadIndexRequest;
                   using ResultType = kvrpcpb::ReadIndexResponse;
+                  static const char* err_msg() {

Contributor

innerr Aug 1, 2019

Why is not errMsg?

Member

hanfei1991 Aug 1, 2019

It will report compile error. Old driver teach me !

contrib/client-c/include/tikv/Rpc.h Outdated

+                  }
+                  static ::grpc::Status doRPCCall(grpc::ClientContext * context, std::unique_ptr<tikvpb::Tikv::Stub> stub, const RequestType & req, ResultType * res) {
+                      return stub -> KvGet(context, req, res);

Contributor

innerr Aug 1, 2019

As above

contrib/client-c/include/tikv/Rpc.h Outdated

+                  using RequestType = kvrpcpb::GetRequest;
+                  using ResultType = kvrpcpb::GetResponse;
+                  static const char* err_msg() {

Contributor

innerr Aug 1, 2019

As above

contrib/client-c/include/tikv/Rpc.h Outdated Show resolved Hide resolved

zanmato1984 and others added 2 commits

August 1, 2019 18:29


          Remove useless commands

d8505b4


          format client-c

510b409

Contributor Author

zanmato1984 commented Aug 2, 2019

/run-integration-tests

innerr reviewed

View reviewed changes

contrib/client-c/include/tikv/Backoff.h

-              struct Backoff {
+              struct Backoff
+              {
                   int base;

Contributor

innerr Aug 3, 2019

size_t may be more accurate, but not a big deal

innerr reviewed

View reviewed changes

contrib/client-c/include/tikv/RegionClient.h

-                  RegionClient(RegionCachePtr cache_, RpcClientPtr client_, const RegionVerID & id) : cache(cache_), client(client_), store_addr("you guess?"), region_id(id), log(&Logger::get("pingcap.tikv")) {}
+                  RegionClient(RegionCachePtr cache_, RpcClientPtr client_, const RegionVerID & id)
+                      : cache(cache_), client(client_), store_addr("you guess?"), region_id(id), log(&Logger::get("pingcap.tikv"))

Contributor

innerr Aug 4, 2019

you guess => a address that can lead to fast failure, should be better.

Contributor

innerr Aug 4, 2019

you guess may cause a long time network searching, stuck till timeout

Member

hanfei1991 Aug 5, 2019

We'd better refine in future

contrib/client-c/include/tikv/Backoff.h

    
                          case EqualJitter:

                              v = expo(base, cap, attempts);

                              sleep_time = v/2 + rand() % (v/2);

                              sleep_time = v / 2 + rand() % (v / 2);

Contributor

innerr Aug 4, 2019

Is is a little jumpy?

Member

hanfei1991 Aug 5, 2019

https://internal.pingcap.net/jira/browse/FLASH-398

contrib/client-c/include/tikv/Backoff.h

                               break;
                           case DecorrJitter:
-                              sleep_time = int(std::min(double(cap), double(base + rand() % (last_sleep*3-base))));
+                              sleep_time = int(std::min(double(cap), double(base + rand() % (last_sleep * 3 - base))));

Contributor

innerr Aug 4, 2019

A comment would be nice, I don't really get it.

Member

hanfei1991 Aug 5, 2019

https://internal.pingcap.net/jira/browse/FLASH-398

contrib/client-c/src/test/get_region_id_wrong_test.cc

    
                  pd_server->stores[1]->setReadIndex(5);

                  pd_server->stores[1]->inject_region_not_found = true;

                  ::sleep(1);

Contributor

innerr Aug 4, 2019

A more graceful waiting method like join?

Member

hanfei1991 Aug 5, 2019

https://internal.pingcap.net/jira/browse/FLASH-397

contrib/client-c/src/tikv/Region.cc

-                      try {
+              RegionPtr RegionCache::loadRegion(Backoffer & bo, std::string key)
+              {
+                  for (;;)

Contributor

innerr Aug 4, 2019

Do we need a stop method? for TiFlash process's graceful exiting

Member

hanfei1991 Aug 5, 2019

https://internal.pingcap.net/jira/browse/FLASH-397

dbms/src/Interpreters/InterpreterSelectQuery.cpp Show resolved Hide resolved

dbms/src/Server/Server.cpp Show resolved Hide resolved

dbms/src/Storages/Transaction/Region.cpp Show resolved Hide resolved

dbms/src/Storages/Transaction/RegionBlockReader.cpp Show resolved Hide resolved

dbms/src/Storages/Transaction/RegionCFDataBase.cpp Show resolved Hide resolved

solotzg reviewed

View reviewed changes

dbms/src/Debug/MockSchemaSyncer.cpp Outdated Show resolved Hide resolved

zanmato1984 added 2 commits

August 5, 2019 13:13


          Merge branch 'ddl' of github.com:pingcap/tics into ddl

f885b95


          Address comments

75d271f

Contributor Author

zanmato1984 commented Aug 5, 2019

/run-integration-tests

hanfei1991 added 2 commits

August 5, 2019 14:36


          address comments

56b594b


          hot fix

688892f

innerr reviewed

View reviewed changes

dbms/src/Storages/Transaction/SchemaBuilder.cpp Show resolved Hide resolved

innerr reviewed

View reviewed changes

dbms/src/Storages/Transaction/SchemaBuilder.cpp

+                      }
+                      default:
+                      {
+                          break;

Contributor

innerr Aug 5, 2019

throw?

Member

hanfei1991 Aug 5, 2019

No need to process other ddl operations.

Contributor Author

zanmato1984 commented Aug 5, 2019

/run-integration-tests

innerr reviewed

View reviewed changes

dbms/src/Storages/Transaction/TiKVVarInt.h

-                      : 10))))))));
+                  return x < (1ULL << 7)
+                      ? 1
+                      : (x < (1ULL << 14)

Contributor

innerr Aug 5, 2019

::cry::

innerr reviewed

View reviewed changes

tests/docker/run.sh

               docker-compose up -d --scale tics0=0 --scale tiflash0=0 --scale tikv-learner0=0
-              sleep 10
+              sleep 60

Contributor

innerr Aug 5, 2019

More graceful checking to avoid this waiting?

Contributor Author

zanmato1984 Aug 5, 2019

Bootstrap process could be tricky as we observed several times that TiKV was NOT ready after several tens of seconds. However TiFlash doesn't have graceful retry for errors during bootstrap - it just bails out. So here we do this workaround by waiting more time.

innerr approved these changes

View reviewed changes

Contributor

innerr left a comment •

edited

Loading

LGTM except for comments and *.test, we can merge this PR after all comments are addressed, I will check *.test later.

zanmato1984 added 2 commits

August 5, 2019 16:38


          Fix failed test

573a569


          Merge branch 'ddl' of github.com:pingcap/tics into ddl

89170bc

Contributor Author

zanmato1984 commented Aug 5, 2019

/run-integration-tests

zanmato1984 and others added 3 commits

August 5, 2019 17:31


          Fix mutable tests by using no schema syncer

274aa5c


          Merge branch 'master' into ddl

81eb519


          fix. optimize background thread pool. (#153)

152aeab

Contributor Author

zanmato1984 commented Aug 5, 2019

/run-integration-tests

zanmato1984 merged commit 71e1b04 into master

zanmato1984 deleted the ddl branch

August 6, 2019 09:27

JaySon-Huang mentioned this pull request

*: Remove useless DataType widen property #6530

Merged

12 tasks

guo-shaoge pushed a commit to guo-shaoge/tiflash that referenced this pull request


          disagg: Fix error when there are empty partitions (pingcap#8221) (pin…

…gcap#110)

close pingcap#8220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet