-
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
fix bug that execution info for non root mpp task is not acurate enough (#1835) #1840
Merged
windtalker
merged 1 commit into
pingcap:release-5.0
from
ti-srebot:release-5.0-eff941f2e2c3
Apr 30, 2021
Merged
fix bug that execution info for non root mpp task is not acurate enough (#1835) #1840
windtalker
merged 1 commit into
pingcap:release-5.0
from
ti-srebot:release-5.0-eff941f2e2c3
Apr 30, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: ti-srebot <[email protected]>
/run-all-tests |
hanfei1991
approved these changes
Apr 30, 2021
JaySon-Huang
pushed a commit
to JaySon-Huang/tiflash
that referenced
this pull request
Jun 8, 2021
Signed-off-by: ti-srebot <[email protected]> Co-authored-by: Flowyi <[email protected]> cherry pick pingcap#1821 to release-5.0 (pingcap#1827) Signed-off-by: ti-srebot <[email protected]> Co-authored-by: xufei <[email protected]> Remove ReplicatedMergeTree family (pingcap#1805) (pingcap#1826) cherry pick pingcap#1835 to release-5.0 (pingcap#1840) Signed-off-by: ti-srebot <[email protected]> Co-authored-by: xufei <[email protected]> Use file path as cache key (pingcap#1852) (pingcap#1862) Lazily initializing DeltaMergeStore (pingcap#1423) (pingcap#1751) (pingcap#1868) Add background gc check thread for DeltaTree storage (pingcap#1742) (pingcap#1828) * cherry pick pingcap#1742 to release-5.0 Signed-off-by: ti-srebot <[email protected]> * fix conflict Co-authored-by: lidezhu <[email protected]> Fix calculate stable property (pingcap#1839) (pingcap#1878) * cherry pick pingcap#1839 to release-5.0 Signed-off-by: ti-srebot <[email protected]> * fix compile and cherry pick a small fix Co-authored-by: lidezhu <[email protected]> Fix client-c mistake setting current_ts to 0 when resolving lock for async commit (pingcap#1856) (pingcap#1870) support constant folding in dbg coprocessor (pingcap#1881) (pingcap#1884) Optimize & Reduce time cost about ci test (pingcap#1849) (pingcap#1894) * reduce time cost about ci test by parallel * add `-DNO_WERROR=ON` to cmake config for release-darwin build * Fix tidb_ghpr_tics_test fail (pingcap#1895) Signed-off-by: Zhigao Tong <[email protected]> Fix panic with feature `compaction filter` in release-5.0 (pingcap#1891) Init store in background task. (pingcap#1843,pingcap#1896) (pingcap#1874) Push down filter on timestamp type column to storage layer (pingcap#1875) (pingcap#1906) * cherry pick pingcap#1875 to release-5.0 Signed-off-by: ti-srebot <[email protected]> * remove irrevalant code Co-authored-by: lidezhu <[email protected]> Fix ExchangeSender: remove duplicated write stream operation (pingcap#1379) (pingcap#1883) cherry pick pingcap#1917 to release-5.0 (pingcap#1923) Signed-off-by: ti-srebot <[email protected]> Co-authored-by: Fu Zhe <[email protected]> Pre-handle SSTFiles to DTFiles when applying snapshots (pingcap#1439) (pingcap#1867) * Add `raft.snapshot.method` in configuration file to control the method of applying snapshots - "block" decode snapshot data as a block - "file1" decode snapshot data as DTFiles (directory mode) [**By default**] * Add a stream `SSTFilesToBlockInputStream` to decode snapshot data into blocks * Add a stream `BoundedSSTFilesToBlockInputStream` to bound the blocks read from SSTFilesToBlockInputStream by column `_tidb_rowid` * Add a stream `SSTFilesToDTFilesOutputStream` that accept `BoundedSSTFilesToBlockInputStream` as input stream to write blocks into DTFiles * Make `STORAGE_FORMAT_CURRENT` to be `STORAGE_FORMAT_V2` by default to support ingest DTFile into DT engine * Fix the bug that the block decoded from SSTFile is not sorted by primary key and version (pingcap#1888) * Fix panic with feature compaction filter with DTFile * Fix ingest write amplification after snapshot apply optimization (pingcap#1913) Co-authored-by: Zhigao Tong <[email protected]> Signed-off-by: JaySon-Huang <[email protected]> Cast int/real as real (pingcap#1911) (pingcap#1928) Fix the problem that old dmfile is not removed atomically (pingcap#1918) (pingcap#1925) Eliminate the table lock between reading,writing and DDL operators for TiFlash (pingcap#1736) (pingcap#1858) * Eliminate the table lock between reading,writing and DDL operators for TiFlash (pingcap#1736) 1. Port the latest `RWLock` for `IStorage` from Clickhouse. "phase-fair rwlocks have been shown to provide more predictable timings compared to task-fair rwlocks for certain workloads (for relatively low share of writes, <25%). Brandenburg B, Anderson J, (2010)" 2. Refactor the lock model for `IStorage` to eliminate the lock between reading, writing, and DDL operations. 3. Acquire table lock by QueryID/threadName instead of function name 4. Tombstone the database after receiving a "drop" DDL from TiDB. Physically drop the databases / tables after gc safe point. 5. Remove some outdated tests under `tests/mutable-test/txn_schema` (all those tests are ported into `tests/delta-merge-test/raft/schema`) * Mark FIXME for conflict codes Conflicts: dbms/src/Flash/Coprocessor/DAGQueryBlockInterpreter.cpp Signed-off-by: JaySon-Huang <[email protected]> cached tics code incase of clone same code repeatedly (pingcap#1994) (pingcap#2001) Add row & bytes threshold config for CompactLog (pingcap#1997) (pingcap#2005) * cherry pick pingcap#1997 to release-5.0 Signed-off-by: ti-srebot <[email protected]> Co-authored-by: Zhigao Tong <[email protected]> Fix the bug that incomplete write batches are not truncated (pingcap#1934) (pingcap#2003) Revert "Push down filter on timestamp type column to storage layer (pingcap#1875) (pingcap#1906)" (pingcap#2010) This reverts commit 630b612. Revert Lazily Init Store (pingcap#2011) * Revert "Init store in background task. (pingcap#1843,pingcap#1896) (pingcap#1874)" This reverts commit 0882461. Conflicts: dbms/src/Storages/StorageDeltaMerge.cpp * format code * Revert "Lazily initializing DeltaMergeStore (pingcap#1423) (pingcap#1751) (pingcap#1868)" This reverts commit bbce050. Conflicts: dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp dbms/src/Storages/StorageDeltaMerge.cpp dbms/src/Storages/StorageDeltaMerge.h Co-authored-by: JinheLin <[email protected]> Co-authored-by: Flowyi <[email protected]> Revert "background gc thread" (pingcap#2015) * Revert "Fix calculate stable property (pingcap#1839) (pingcap#1878)" This reverts commit 6a9eb58. * Revert "Add background gc check thread for DeltaTree storage (pingcap#1742) (pingcap#1828)" This reverts commit cbbbd09. * remove code * format code * format code * fix test compile * fix comment Fix the potential concurrency problem when clone the shared delta index (pingcap#2030) (pingcap#2033) * cherry pick pingcap#2030 to release-5.0 Signed-off-by: ti-srebot <[email protected]> * Update dbms/src/Storages/DeltaMerge/DeltaIndex.h Co-authored-by: JaySon <[email protected]> Co-authored-by: lidezhu <[email protected]> Co-authored-by: JaySon <[email protected]> Fix bug for unexpected deleted ingest file (pingcap#2047) (pingcap#2048) * Only enable GC for DTFiles after they get applied to all segments. * Refine some loggings Signed-off-by: JaySon-Huang <[email protected]> Fix cast int as time (pingcap#1788) (pingcap#1893) cherry pick pingcap#1903 to release-5.0 (pingcap#1915) Signed-off-by: ti-srebot <[email protected]> Fix ingesting file may add ref page to deleted page (pingcap#2054) (pingcap#2055) 1. Put the ingest file id into `storage_pool.data` before ingesting them into segments 2. Add ref pages to the ingest files for each segment 3. Delete the original page id after all Signed-off-by: JaySon-Huang <[email protected]> Fix compile error on Mac Clang 12.0.5 (pingcap#2058) (pingcap#2070) cherry pick pingcap#2017 pingcap#2020 pingcap#2051 to release-5.0 (pingcap#2074) Revert "cherry pick pingcap#2017 pingcap#2020 pingcap#2051 to release-5.0 (pingcap#2074)" This reverts commit 8e0712c. Revert "Fix compile error on Mac Clang 12.0.5 (pingcap#2058) (pingcap#2070)" This reverts commit 3c31b92. Revert "Fix ingesting file may add ref page to deleted page (pingcap#2054) (pingcap#2055)" This reverts commit c773d61. Revert "Fix bug for unexpected deleted ingest file (pingcap#2047) (pingcap#2048)" This reverts commit b329618. Revert "Pre-handle SSTFiles to DTFiles when applying snapshots (pingcap#1439) (pingcap#1867)" This reverts commit c947fd6. Conflicts: dbms/src/Common/FailPoint.cpp dbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.h dbms/src/Storages/DeltaMerge/File/DMFileWriter.h dbms/src/Storages/DeltaMerge/ReorganizeBlockInputStream.h dbms/src/Storages/DeltaMerge/SSTFilesToBlockInputStream.cpp dbms/src/Storages/DeltaMerge/SSTFilesToBlockInputStream.h dbms/src/Storages/DeltaMerge/SSTFilesToDTFilesOutputStream.cpp dbms/src/Storages/DeltaMerge/Segment.cpp dbms/src/Storages/DeltaMerge/StableValueSpace.cpp dbms/src/Storages/DeltaMerge/tests/gtest_dm_file.cpp dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp dbms/src/Storages/StorageDeltaMerge.cpp dbms/src/Storages/StorageDeltaMerge.h dbms/src/Storages/Transaction/ApplySnapshot.cpp dbms/src/Storages/Transaction/PartitionStreams.cpp tests/delta-merge-test/raft/schema/drop_on_restart.test Fix comment Signed-off-by: JaySon-Huang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
CHERRY-PICK
cherry pick
status/LGT1
Indicates that a PR has LGTM 1.
type/enhancement
The issue or PR belongs to an enhancement.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cherry-pick #1835 to release-5.0
You can switch your code base to this Pull Request by using git-extras:
# In tics repo: git pr https://github.com/pingcap/tics/pull/1840
After apply modifications, you can push your change to this PR via:
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
In current implementation, the execution info for non root mpp task is merged in root mpp task, however, the merge algrithm is not good enough so sometimes the execution time of non root mpp task is very misleading, for example:
The execution time of mpp task end with
ExchangeSender_17
is very short, which may make the user think the data transform fromExchangeSender_17
toExchangeReceiver_18
cost most of the time. However after a more careful debug, I foundExchangeSender_17
costs at least 18sWhat is changed and how it works?
Proposal: xxx
What's Changed:
Some backgrounds:
execution_summaries
, the execution summary includesexecution time
,processed rows
,processed iters
andconcurrency
execution_summaries
returned by TiFlash/TiKV, it is not aware of whether the rpc call is unray or streaming, it treats all the responses as the unary call reponse, which means for processed rows, it will add all the processed rows inexecution_sumamries
as the total processed rows.execution_summaries
, the data will be amplified in TiDB, so in order to make the data more acurate in TiDB side, for a streaming call, we always return a deltaexecution_summaries
TiRemoteBlockInputStream
to receive the response from the non root mpp task. After decoded the response,TiRemoteBlockInputStream
will merge theexecution_summaries
. When merge theexecution_summaries
for the streaming call, it merges thedelta execution_summaries
explicitly.The problem is: Although
TiRemoteBlockInputStream
merges thedelta execution_summaries
explicitly, it just merge part of the streaming call's response, because when reading data from remote, we usually use multipleTiRemoteBlockInputStream
to speed up the query, so everyTiRemoteBlockInputStream
only receive part of the execution info for the non root mpp task, which makes it much more less than the orginal ones.This pr fix it by supporting
streaming execution_summaries
inTiRemoteBlockInputStream
, so for a non root mpp task, it just return the orignalexecution_summaries
, andTiRemoteBlockInputStream
will handle it correctly, however, since TiDB still not supportstreaming execution_summaries
, TiFlash will return deltaexecution_summaries
if the receiver side is TiDB. In the future, after TiDB supportstreaming execution_summaries
, we can removedelta execution summaries
totally.After this pr, the above example will get the following output:
As we can see, the execution time of
ExchangeSender_17
is 19.7s, which is much more close to the actual execution time.Related changes
pingcap/docs
/pingcap/docs-cn
:Check List
Tests
Side effects
Release note