-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FLASH-275/276/277/278/281/282: Complete DDL implementation #110
Conversation
…c service and do init sync on startup
* DDL read and update by tikv's meta * add dbg back * refine test * address comments
/run-integration-tests |
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stub -> ReadIndex
=> stub->ReadIndex
contrib/client-c/include/tikv/Rpc.h
Outdated
using ResultType = kvrpcpb::ReadIndexResponse; | ||
|
||
static const char* err_msg() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is not errMsg
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
contrib/client-c/include/tikv/Rpc.h
Outdated
using RequestType = kvrpcpb::GetRequest; | ||
using ResultType = kvrpcpb::GetResponse; | ||
|
||
static const char* err_msg() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
/run-integration-tests |
|
||
struct Backoff { | ||
struct Backoff | ||
{ | ||
int base; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t may be more accurate, but not a big deal
|
||
Logger * log; | ||
|
||
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")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you guess
=> a address that can lead to fast failure, should be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you guess
may cause a long time network searching, stuck till timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd better refine in future
@@ -59,31 +66,34 @@ struct Backoff { | |||
break; | |||
case EqualJitter: | |||
v = expo(base, cap, attempts); | |||
sleep_time = v/2 + rand() % (v/2); | |||
sleep_time = v / 2 + rand() % (v / 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is is a little jumpy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment would be nice, I don't really get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pd_server -> stores[1] -> inject_region_not_found = true; | ||
pd_server->addRegion(region, 0, 1); | ||
pd_server->stores[1]->setReadIndex(5); | ||
pd_server->stores[1]->inject_region_not_found = true; | ||
|
||
::sleep(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more graceful waiting method like join
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try { | ||
RegionPtr RegionCache::loadRegion(Backoffer & bo, std::string key) | ||
{ | ||
for (;;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a stop
method? for TiFlash process's graceful exiting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/run-integration-tests |
} | ||
default: | ||
{ | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to process other ddl operations.
/run-integration-tests |
: 10)))))))); | ||
return x < (1ULL << 7) | ||
? 1 | ||
: (x < (1ULL << 14) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
::cry::
@@ -10,7 +10,7 @@ rm -rf ./data ./log | |||
|
|||
docker-compose up -d --scale tics0=0 --scale tiflash0=0 --scale tikv-learner0=0 | |||
|
|||
sleep 10 | |||
sleep 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More graceful checking to avoid this waiting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for comments and *.test, we can merge this PR after all comments are addressed, I will check *.test later.
/run-integration-tests |
/run-integration-tests |
Doc links: design doc, test doc
There are several relative individual parts of this huge PR:
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
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).
5.1 Lots of debug function for schema syncing and mock TiDB table
5.2 Lots of tests using mock TiDB and schema syncer