Fix ExchangeSender: remove duplicated write stream operation #1379

leiysky · 2021-01-25T08:10:27Z

What problem does this PR solve?

Problem Summary:
getEncodePartitionTask will write same chunk for multiple times.

What is changed and how it works?

What's Changed:

Remove duplicated writer->write operation

How it Works:

Related changes

Need to cherry-pick to the release branch

Check List

Tests

Manual test (add detailed scripts or steps below)

Release note

Fix problem ExchangeSender will sometimes send duplicated data chunks

windtalker · 2021-01-25T09:02:04Z

@fzhedu PTAL

leiysky · 2021-01-25T10:23:12Z

/run-all-tests

leiysky · 2021-01-26T03:39:51Z

/run-all-tests

JaySon-Huang · 2021-01-27T13:44:39Z

/run-all-tests

fzhedu

could you test the correctness of this PR? such as running TPCH

fzhedu

how does this PR improve performance? we can also adjust records_per_chunk to see its effects.

leiysky · 2021-01-28T09:17:29Z

could you test the correctness of this PR? such as running TPCH

It's hard to reproduce, since there is always only one block for each encode task, in which case every response will be written for once and won't produce duplicated chunks. Besides, we can hardly find out whether the correctness issue is fixed by this PR or not, since we don't have correctness test CI for now.

By the way, apparently original logic is wrong. We add chunk to response and write response to MPPTunnel in every loop without clear the response. It means the chunks added in previous loops will be send for times.

how does this PR improve performance? we can also adjust records_per_chunk to see its effects.

This PR is to fix a correctness issue, and won't improve performance. CHBlockChunkCodec cannot batch blocks, which means increasing records_per_chunk will only batch numbers of Block but won't merge them.

I'll implement sender batching in another PR.

leiysky · 2021-01-28T09:30:18Z

/run-all-tests

leiysky · 2021-01-29T06:37:28Z

/run-all-tests

fzhedu · 2021-02-04T04:41:26Z

LGTM

ti-srebot · 2021-02-04T04:41:29Z

@fzhedu, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIG: tiflash(slack).

leiysky · 2021-05-07T01:53:08Z

/merge

ti-srebot · 2021-05-07T01:53:38Z

/run-all-tests

ti-srebot · 2021-05-07T03:03:12Z

@leiysky merge failed.

leiysky · 2021-05-10T06:51:56Z

/merge

ti-srebot · 2021-05-10T06:54:24Z

/run-all-tests

ti-srebot · 2021-05-10T07:36:23Z

@leiysky merge failed.

leiysky · 2021-05-10T07:37:28Z

/merge

ti-srebot · 2021-05-10T07:37:30Z

Your auto merge job has been accepted, waiting for:

1876

ti-srebot · 2021-05-10T08:32:53Z

/run-all-tests

ti-srebot · 2021-05-10T10:22:49Z

cherry pick to release-5.0 in PR #1883

…1883)

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]>

fix duplicated writing gRPC stream

10441cc

leiysky added type/bugfix This PR fixes a bug. needs-cherry-pick-release-5.0-rc PR which needs to be cherry-picked to release-5.0-rc labels Jan 25, 2021

leiysky requested a review from windtalker January 25, 2021 08:10

windtalker requested a review from fzhedu January 25, 2021 09:02

Merge branch 'master' into fix/duplicated-write-stream

5772d5a

Merge branch 'master' into fix/duplicated-write-stream

371b3a5

JaySon-Huang removed the needs-cherry-pick-release-5.0-rc PR which needs to be cherry-picked to release-5.0-rc label Jan 27, 2021

Merge branch 'master' into fix/duplicated-write-stream

2d035a2

fzhedu reviewed Jan 28, 2021

View reviewed changes

Merge branch 'master' into fix/duplicated-write-stream

d22a281

Merge branch 'master' into fix/duplicated-write-stream

5b18f41

Merge branch 'master' into fix/duplicated-write-stream

a033c4f

hanfei1991 approved these changes May 7, 2021

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label May 7, 2021

Merge branch 'master' into fix/duplicated-write-stream

bcb7145

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label May 7, 2021

Merge branch 'master' into fix/duplicated-write-stream

c1058ad

leiysky added the needs-cherry-pick-release-5.0 PR which needs to be cherry-picked to release-5.0 label May 10, 2021

Merge branch 'master' into fix/duplicated-write-stream

f3fed0e

ti-srebot merged commit 2199fe1 into pingcap:master May 10, 2021

ti-srebot mentioned this pull request May 10, 2021

Fix ExchangeSender: remove duplicated write stream operation (#1379) #1883

Merged

ti-srebot added a commit that referenced this pull request May 18, 2021

Fix ExchangeSender: remove duplicated write stream operation (#1379) (#…

ae8db94

…1883)

JaySon-Huang mentioned this pull request Jun 8, 2021

Check the diff between revert_snapshot and pick need commit JaySon-Huang/tiflash#8

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ExchangeSender: remove duplicated write stream operation #1379

Fix ExchangeSender: remove duplicated write stream operation #1379

leiysky commented Jan 25, 2021

windtalker commented Jan 25, 2021

leiysky commented Jan 25, 2021

leiysky commented Jan 26, 2021

JaySon-Huang commented Jan 27, 2021

fzhedu left a comment

fzhedu left a comment

leiysky commented Jan 28, 2021

leiysky commented Jan 28, 2021

leiysky commented Jan 29, 2021

fzhedu commented Feb 4, 2021

ti-srebot commented Feb 4, 2021

leiysky commented May 7, 2021

ti-srebot commented May 7, 2021

ti-srebot commented May 7, 2021

leiysky commented May 10, 2021

ti-srebot commented May 10, 2021

ti-srebot commented May 10, 2021

leiysky commented May 10, 2021

ti-srebot commented May 10, 2021

ti-srebot commented May 10, 2021

ti-srebot commented May 10, 2021

Fix ExchangeSender: remove duplicated write stream operation #1379

Fix ExchangeSender: remove duplicated write stream operation #1379

Conversation

leiysky commented Jan 25, 2021

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

windtalker commented Jan 25, 2021

leiysky commented Jan 25, 2021

leiysky commented Jan 26, 2021

JaySon-Huang commented Jan 27, 2021

fzhedu left a comment

Choose a reason for hiding this comment

fzhedu left a comment

Choose a reason for hiding this comment

leiysky commented Jan 28, 2021

leiysky commented Jan 28, 2021

leiysky commented Jan 29, 2021

fzhedu commented Feb 4, 2021

ti-srebot commented Feb 4, 2021

leiysky commented May 7, 2021

ti-srebot commented May 7, 2021

ti-srebot commented May 7, 2021

leiysky commented May 10, 2021

ti-srebot commented May 10, 2021

ti-srebot commented May 10, 2021

leiysky commented May 10, 2021

ti-srebot commented May 10, 2021

ti-srebot commented May 10, 2021

ti-srebot commented May 10, 2021