Skip to content
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

Bugfix: schrodinger bank2 fail #521

Merged
merged 11 commits into from
Mar 17, 2020
Merged

Bugfix: schrodinger bank2 fail #521

merged 11 commits into from
Mar 17, 2020

Conversation

flowbehappy
Copy link
Contributor

@flowbehappy flowbehappy commented Mar 12, 2020

  • Flush committed data in Region after resolve locks
  • Stop append into last packs after split.
  • Remove last_cache in Delta to reduce code complexity.
  • Add system table: dt_tables and dt_segments, for debug.

JaySon-Huang
JaySon-Huang previously approved these changes Mar 12, 2020
Copy link
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JaySon-Huang JaySon-Huang changed the title DT: Flush region after resolve locks [DNM] DT: Flush region after resolve locks Mar 12, 2020
@JaySon-Huang JaySon-Huang dismissed their stale review March 12, 2020 12:38

Not work.

@JaySon-Huang JaySon-Huang changed the title [DNM] DT: Flush region after resolve locks DT: Flush region after resolve locks Mar 13, 2020
@flowbehappy flowbehappy changed the title DT: Flush region after resolve locks Bugfix: schrodinger bank2 fail Mar 16, 2020
@flowbehappy
Copy link
Contributor Author

@JaySon-Huang PTAL

@JaySon-Huang
Copy link
Contributor

@solotzg take a look at dbms/src/Storages/Transaction/PartitionStreams.cpp?

@flowbehappy flowbehappy requested a review from solotzg March 17, 2020 06:53
@@ -65,7 +38,6 @@ void RegionTable::writeBlockByRegion(Context & context, RegionPtr region, Region
return false;
// Table must have just been dropped or truncated.
// TODO: What if we support delete range? Do we still want to remove KVs from region cache?
data_list_to_remove = std::move(data_list_read);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why break logic here

Copy link
Contributor Author

@flowbehappy flowbehappy Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to here : https://github.com/pingcap/tics/pull/521/files/076d52a4d411e5d3e9e8868aeba9517d44902bcf#diff-a340ae21d5c05e7ec818cbfd7b0bcfedR164

Here I extract the "write to storage" logic into the new function writeDataToStorage

Copy link
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@flowbehappy flowbehappy requested a review from zanmato1984 March 17, 2020 10:04
@flowbehappy
Copy link
Contributor Author

@zanmato1984 PTAL

@@ -116,6 +116,16 @@ class RegionTable : private boost::noncopyable
RegionDataReadInfoList tryFlushRegion(RegionID region_id, bool try_persist = false);
RegionDataReadInfoList tryFlushRegion(const RegionPtr & region, bool try_persist);

static RegionException::RegionReadStatus resolveLocksAndFlushRegion(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put it together with writeBlockByRegion and readBlockByRegion, and add proper comments as the other two.

@flowbehappy
Copy link
Contributor Author

@zanmato1984 PTAL

Copy link
Contributor

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@flowbehappy
Copy link
Contributor Author

/run-integration-tests

@flowbehappy flowbehappy merged commit a391520 into pingcap:master Mar 17, 2020
@JaySon-Huang JaySon-Huang added the needs-cherry-pick-release-3.1 PR which needs to be cherry-picked to release-3.1 label Mar 17, 2020
@JaySon-Huang
Copy link
Contributor

/run-cherry-picker

@sre-bot
Copy link
Collaborator

sre-bot commented Mar 17, 2020

cherry pick to release-3.1 in PR #528

JaySon-Huang pushed a commit that referenced this pull request Mar 18, 2020
* Flush committed data in Region after resolve locks
* Stop append into last packs after split.
* Remove last_cache in Delta to reduce code complexity.
* Add system table: dt_tables and dt_segments, for debug.

Co-authored-by: flow <[email protected]>
@JaySon-Huang JaySon-Huang deleted the flush-region-after-resolvelocks branch March 20, 2020 12:25
windtalker added a commit that referenced this pull request Mar 26, 2020
* fix daily test fail (#520)

* fix daily test fail

* fix

* Add fullstack test for engine DeltaTree (#524)

## Add fullstack test for engine DeltaTree.
* Refine `tests/docker/run.sh` and split `tests/docker/docker-compose.yaml` into `tests/docker/{gtest/mock-test/cluster/tiflash-dt/tiflash-tmt}.yaml`

`fullstack/ddl`,`fullstack-test/fault-inject` will be enabled in #526 

## Others
* Add column `tidb_table_id` in `system.tables`
* Add some debugging info

Signed-off-by: JaySon-Huang <[email protected]>

* Bugfix: schrodinger bank2 fail (#521)

* Flush committed data in Region after resolve locks
* Stop append into last packs after split.
* Remove last_cache in Delta to reduce code complexity.
* Add system table: dt_tables and dt_segments, for debug.

* [FLASH-1008] Support br restore & ingest sst (#529)

* [flash-1018]fix bug of datetime default value (#534)

* fix bug of datetime default value

* address comment

* Using SegmentSnapshotPtr instead of SegmentSnapshot (#532)

* [Flash-664] Enable DDL for engine DeltaTree (#526)

- [x] Enable unittest in gtest_dbms
- [x] Enable mock test in tests/delta-merge-test
- [x] Enable fullstack-test/ddl
- [x] Enable fullstack-test/inject (Imported in #443)
- [x] Refine exception while read / write to DeltaTree (FLASH-994)
  * Use `Exception::addMessage` to add more diagnostics for locate which table is wrong (commit: 716ae4a)
- [x] shutdown should cancel all background tasks (FLASH-995) (commit: 7470c2f)
- [x] Run schrodinger/sddl test

## Others 
* Add atomic-rename table test in `tests/fullstack-test/fault-inject/rename-table.test`, but did not enable this. We will fix it later.
* "dt" engine ONLY support disable_bg_flush = true.
If background flush is enabled, read will not triggle schema sync. Which means that we may not get the right result with out-dated schema.
* Found that 'zero' value of type year is not match with tikv (FLASH-1023)

Signed-off-by: JaySon-Huang <[email protected]>

* [FLASH-1027] Fix: proxy override system signal listening (#541)

* remove signal listening from proxy.

* while terminating, stop all learner read.

* [FLASH-1026] Synchronization between drop table and remove region (#538)

* Set default storage engine to DT (#547)

* Do region flush in Region::handleWriteRaftCmd (#542)

* [FLASH-1028] Region merge should not remove data (#544)

* Remove region after region merge should not remove data

Signed-off-by: JaySon-Huang <[email protected]>

* Fix region init index

Signed-off-by: JaySon-Huang <[email protected]>

* Add region_merge.test for DT

Signed-off-by: JaySon-Huang <[email protected]>

* Fix different behavior between DT and TMT

Signed-off-by: JaySon-Huang <[email protected]>

* Fix mock remove region

Signed-off-by: JaySon-Huang <[email protected]>

Co-authored-by: pingcap-github-bot <[email protected]>

* clean useless code

Co-authored-by: JaySon <[email protected]>
Co-authored-by: Flowyi <[email protected]>
Co-authored-by: Tong Zhigao <[email protected]>
Co-authored-by: Han Fei <[email protected]>
Co-authored-by: pingcap-github-bot <[email protected]>
hanfei1991 added a commit that referenced this pull request Jun 22, 2020
* implement join

* fix

* update tipb

* update tipb

* save work: refine dag interpreter, introduce dag interpreter query block

* fix bug

* comment useless code

* refine code

* fix bug

* refine code

* fix bug

* save work

* tiny fix

* save work

* support remote read

* update tipb

* refine code

* fix bug

* update client-c

* refine cop read

* update client-c to support cop reading from tiflash

* refine code

* support batch cop

* fix build error

* fix daily test fail

* some bug fix

* log dag execution time without encode to chunk

* fix bug

* fix bug

* log dag execution time without encode

* make encode multi processors

* fix

* parallel encode

* refine code of batch coprocessor

* refine code

* delete useless code

* update kvproto and client-c

* [flash 1002]refine coprocessor read (#530)

* refind coprocessor

* fix

* try fix ci

* support key ranges in batch coprocessor (#533)

* save work

* save work

* save work

* save work

* dt support key ranges in dag request

* fix bug

* fix bug

* fix bug

* add some comments

* fix bug

* address comments

* merge master branch (#556)

* fix daily test fail (#520)

* fix daily test fail

* fix

* Add fullstack test for engine DeltaTree (#524)

## Add fullstack test for engine DeltaTree.
* Refine `tests/docker/run.sh` and split `tests/docker/docker-compose.yaml` into `tests/docker/{gtest/mock-test/cluster/tiflash-dt/tiflash-tmt}.yaml`

`fullstack/ddl`,`fullstack-test/fault-inject` will be enabled in #526 

## Others
* Add column `tidb_table_id` in `system.tables`
* Add some debugging info

Signed-off-by: JaySon-Huang <[email protected]>

* Bugfix: schrodinger bank2 fail (#521)

* Flush committed data in Region after resolve locks
* Stop append into last packs after split.
* Remove last_cache in Delta to reduce code complexity.
* Add system table: dt_tables and dt_segments, for debug.

* [FLASH-1008] Support br restore & ingest sst (#529)

* [flash-1018]fix bug of datetime default value (#534)

* fix bug of datetime default value

* address comment

* Using SegmentSnapshotPtr instead of SegmentSnapshot (#532)

* [Flash-664] Enable DDL for engine DeltaTree (#526)

- [x] Enable unittest in gtest_dbms
- [x] Enable mock test in tests/delta-merge-test
- [x] Enable fullstack-test/ddl
- [x] Enable fullstack-test/inject (Imported in #443)
- [x] Refine exception while read / write to DeltaTree (FLASH-994)
  * Use `Exception::addMessage` to add more diagnostics for locate which table is wrong (commit: 716ae4a)
- [x] shutdown should cancel all background tasks (FLASH-995) (commit: 7470c2f)
- [x] Run schrodinger/sddl test

## Others 
* Add atomic-rename table test in `tests/fullstack-test/fault-inject/rename-table.test`, but did not enable this. We will fix it later.
* "dt" engine ONLY support disable_bg_flush = true.
If background flush is enabled, read will not triggle schema sync. Which means that we may not get the right result with out-dated schema.
* Found that 'zero' value of type year is not match with tikv (FLASH-1023)

Signed-off-by: JaySon-Huang <[email protected]>

* [FLASH-1027] Fix: proxy override system signal listening (#541)

* remove signal listening from proxy.

* while terminating, stop all learner read.

* [FLASH-1026] Synchronization between drop table and remove region (#538)

* Set default storage engine to DT (#547)

* Do region flush in Region::handleWriteRaftCmd (#542)

* [FLASH-1028] Region merge should not remove data (#544)

* Remove region after region merge should not remove data

Signed-off-by: JaySon-Huang <[email protected]>

* Fix region init index

Signed-off-by: JaySon-Huang <[email protected]>

* Add region_merge.test for DT

Signed-off-by: JaySon-Huang <[email protected]>

* Fix different behavior between DT and TMT

Signed-off-by: JaySon-Huang <[email protected]>

* Fix mock remove region

Signed-off-by: JaySon-Huang <[email protected]>

Co-authored-by: pingcap-github-bot <[email protected]>

* clean useless code

Co-authored-by: JaySon <[email protected]>
Co-authored-by: Flowyi <[email protected]>
Co-authored-by: Tong Zhigao <[email protected]>
Co-authored-by: Han Fei <[email protected]>
Co-authored-by: pingcap-github-bot <[email protected]>

* fix type mismatch bug in broadcast join

* broadcast join support join keys with different data type (#580)

* fix type mismatch bug in broadcast join

* refine code

* refine code

* some improvement for broadcast join (#600)

* some improvement for broadcast join

* format code

* refine code

* address comment

* fix bug

* address comments

* fix bug

* fix bug

* make TiFlash backward compatible to old tipb (#653)

* 1. re-enable exec info in dag response, 2. support old style dag request

* basic support for execute summary

* refine support of executor time for join plan

* format code

* address comments

* fix bug

* refine code

* update header

* Fix execute details regression after merge master (#678)

* refine code

* fix bug

* fix bug

* format code

* update kvproto

* fmt code

* update client-c

* update tipb

Co-authored-by: xufei <[email protected]>
Co-authored-by: xufei <[email protected]>
Co-authored-by: JaySon <[email protected]>
Co-authored-by: Flowyi <[email protected]>
Co-authored-by: Tong Zhigao <[email protected]>
Co-authored-by: pingcap-github-bot <[email protected]>
hanfei1991 added a commit that referenced this pull request Jul 2, 2020
* implement join

* fix

* update tipb

* update tipb

* save work: refine dag interpreter, introduce dag interpreter query block

* fix bug

* comment useless code

* refine code

* fix bug

* refine code

* fix bug

* save work

* tiny fix

* save work

* support remote read

* update tipb

* refine code

* fix bug

* update client-c

* refine cop read

* update client-c to support cop reading from tiflash

* refine code

* support batch cop

* fix build error

* fix daily test fail

* some bug fix

* log dag execution time without encode to chunk

* fix bug

* fix bug

* log dag execution time without encode

* make encode multi processors

* fix

* parallel encode

* refine code of batch coprocessor

* refine code

* delete useless code

* update kvproto and client-c

* [flash 1002]refine coprocessor read (#530)

* refind coprocessor

* fix

* try fix ci

* support key ranges in batch coprocessor (#533)

* save work

* save work

* save work

* save work

* dt support key ranges in dag request

* fix bug

* fix bug

* fix bug

* add some comments

* fix bug

* address comments

* merge master branch (#556)

* fix daily test fail (#520)

* fix daily test fail

* fix

* Add fullstack test for engine DeltaTree (#524)

## Add fullstack test for engine DeltaTree.
* Refine `tests/docker/run.sh` and split `tests/docker/docker-compose.yaml` into `tests/docker/{gtest/mock-test/cluster/tiflash-dt/tiflash-tmt}.yaml`

`fullstack/ddl`,`fullstack-test/fault-inject` will be enabled in #526 

## Others
* Add column `tidb_table_id` in `system.tables`
* Add some debugging info

Signed-off-by: JaySon-Huang <[email protected]>

* Bugfix: schrodinger bank2 fail (#521)

* Flush committed data in Region after resolve locks
* Stop append into last packs after split.
* Remove last_cache in Delta to reduce code complexity.
* Add system table: dt_tables and dt_segments, for debug.

* [FLASH-1008] Support br restore & ingest sst (#529)

* [flash-1018]fix bug of datetime default value (#534)

* fix bug of datetime default value

* address comment

* Using SegmentSnapshotPtr instead of SegmentSnapshot (#532)

* [Flash-664] Enable DDL for engine DeltaTree (#526)

- [x] Enable unittest in gtest_dbms
- [x] Enable mock test in tests/delta-merge-test
- [x] Enable fullstack-test/ddl
- [x] Enable fullstack-test/inject (Imported in #443)
- [x] Refine exception while read / write to DeltaTree (FLASH-994)
  * Use `Exception::addMessage` to add more diagnostics for locate which table is wrong (commit: 716ae4a)
- [x] shutdown should cancel all background tasks (FLASH-995) (commit: 7470c2f)
- [x] Run schrodinger/sddl test

## Others 
* Add atomic-rename table test in `tests/fullstack-test/fault-inject/rename-table.test`, but did not enable this. We will fix it later.
* "dt" engine ONLY support disable_bg_flush = true.
If background flush is enabled, read will not triggle schema sync. Which means that we may not get the right result with out-dated schema.
* Found that 'zero' value of type year is not match with tikv (FLASH-1023)

Signed-off-by: JaySon-Huang <[email protected]>

* [FLASH-1027] Fix: proxy override system signal listening (#541)

* remove signal listening from proxy.

* while terminating, stop all learner read.

* [FLASH-1026] Synchronization between drop table and remove region (#538)

* Set default storage engine to DT (#547)

* Do region flush in Region::handleWriteRaftCmd (#542)

* [FLASH-1028] Region merge should not remove data (#544)

* Remove region after region merge should not remove data

Signed-off-by: JaySon-Huang <[email protected]>

* Fix region init index

Signed-off-by: JaySon-Huang <[email protected]>

* Add region_merge.test for DT

Signed-off-by: JaySon-Huang <[email protected]>

* Fix different behavior between DT and TMT

Signed-off-by: JaySon-Huang <[email protected]>

* Fix mock remove region

Signed-off-by: JaySon-Huang <[email protected]>

Co-authored-by: pingcap-github-bot <[email protected]>

* clean useless code

Co-authored-by: JaySon <[email protected]>
Co-authored-by: Flowyi <[email protected]>
Co-authored-by: Tong Zhigao <[email protected]>
Co-authored-by: Han Fei <[email protected]>
Co-authored-by: pingcap-github-bot <[email protected]>

* fix type mismatch bug in broadcast join

* broadcast join support join keys with different data type (#580)

* fix type mismatch bug in broadcast join

* refine code

* refine code

* some improvement for broadcast join (#600)

* some improvement for broadcast join

* format code

* refine code

* address comment

* fix bug

* address comments

* fix bug

* fix bug

* make TiFlash backward compatible to old tipb (#653)

* 1. re-enable exec info in dag response, 2. support old style dag request

* basic support for execute summary

* refine support of executor time for join plan

* format code

* address comments

* fix bug

* refine code

* update header

* Fix execute details regression after merge master (#678)

* refine code

* fix bug

* fix bug

* format code

* update kvproto

* fmt code

* update client-c

* update tipb

Co-authored-by: xufei <[email protected]>
Co-authored-by: xufei <[email protected]>
Co-authored-by: JaySon <[email protected]>
Co-authored-by: Flowyi <[email protected]>
Co-authored-by: Tong Zhigao <[email protected]>
Co-authored-by: pingcap-github-bot <[email protected]>

Co-authored-by: xufei <[email protected]>
Co-authored-by: xufei <[email protected]>
Co-authored-by: JaySon <[email protected]>
Co-authored-by: Flowyi <[email protected]>
Co-authored-by: Tong Zhigao <[email protected]>
Co-authored-by: pingcap-github-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cherry-pick-release-3.1 PR which needs to be cherry-picked to release-3.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants