Adapt to the blocklist feature in serverless by CalvinNeo · Pull Request #9578 · pingcap/tiflash

CalvinNeo · 2024-11-01T08:56:44Z

Issue Number: close #9575

Problem Summary:

The feature is not enabled on master branch.

See https://github.com/tidbcloud/tiflash-cse/pull/308 for more.

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

JinheLin · 2024-11-04T03:37:06Z

dbms/src/Interpreters/Settings.h

    M(SettingUInt64, disagg_build_task_timeout, DEFAULT_DISAGG_TASK_BUILD_TIMEOUT_SEC, "disagg task establish timeout, unit is second.")                                                                                                \
    M(SettingUInt64, disagg_task_snapshot_timeout, DEFAULT_DISAGG_TASK_TIMEOUT_SEC, "disagg task snapshot max endurable time, unit is second.")                                                                                         \
    M(SettingUInt64, disagg_fetch_pages_timeout, DEFAULT_DISAGG_FETCH_PAGES_TIMEOUT_SEC, "fetch disagg pages timeout for one segment, unit is second.")                                                                                 \
+    M(SettingString, disagg_blocklist_wn_store_id, "", "comma seperated unsigned integers representing `store_id`s of stores that are blocklisted.")                                                                                    \


Is this parameter designed to identify issues online and configure them manually?

Lloyd-Pottiger

lgtm

JinheLin · 2024-11-04T03:40:48Z

dbms/src/Storages/StorageDisaggregated.cpp

-{}
+{
+#if SERVERLESS_PROXY == 1
+    std::istringstream iss(context.getSettingsRef().disagg_blocklist_wn_store_id);


Why not parse this parameter when the server startup?

JinheLin · 2024-11-04T03:42:01Z

dbms/src/Storages/StorageDisaggregated.cpp

+    std::istringstream iss(context.getSettingsRef().disagg_blocklist_wn_store_id);
+    std::string token;
+
+    while (std::getline(iss, token, ','))


Using functions like boost::split can simplify these code.

Seems the function is not applicable to unordereed_set

JinheLin · 2024-11-04T03:43:22Z

dbms/src/Storages/StorageDisaggregated.h

    const FilterConditions & filter_conditions;

    std::unique_ptr<DAGExpressionAnalyzer> analyzer;
+    std::unordered_set<uint64_t> store_id_blocklist;


I think store_id_blocklist is very small, and std::vector is more cache friendly and efficently.

It will eventually be converted to unordered_set as an argument to a API in client-c. A better way(if necessay) is make store_id_blocklist into a sigleton.

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

…dapt-to-master

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

dbms/src/Interpreters/Context.h

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

dbms/src/Interpreters/Context.cpp

Co-authored-by: JaySon <tshent@qq.com>

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

…dapt-to-master

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

JaySon-Huang

LGTM

ti-chi-bot · 2024-11-06T06:37:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JaySon-Huang,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-11-06T06:37:03Z

[LGTM Timeline notifier]

Timeline:

2024-11-04 03:37:15.453906349 +0000 UTC m=+839348.293061888: ☑️ agreed by Lloyd-Pottiger.
2024-11-06 06:37:03.185697677 +0000 UTC m=+1022936.024853217: ☑️ agreed by JaySon-Huang.

CalvinNeo · 2024-11-06T07:45:17Z


tidb-ci/force_enable_lm/timestamp_literal.test: Running

  File: tidb-ci/force_enable_lm/timestamp_literal.test

  Error line: 22

  Error: set @@tidb_isolation_read_engines='tiflash'; set time_zone = '+9:00'; select * from test.t where value = (select max(value) from test.t);

  Result:

    ERROR 1105 (HY000) at line 1: interface conversion: interface is nil, not base.Plan

  Expected:

    +------+---------------------+

    | id   | value               |

    +------+---------------------+

    |    1 | 2020-01-01 09:00:00 |

    +------+---------------------+

CalvinNeo · 2024-11-06T07:45:27Z

/pull-integration-test

CalvinNeo · 2024-11-07T02:40:08Z

/pull-integration-test

JaySon-Huang · 2024-11-07T02:50:45Z

/test pull-integration-test

close pingcap#9575 Signed-off-by: Calvin Neo <calvinneo1995@gmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

JaySon-Huang · 2024-11-18T15:47:57Z

cherry pick to release-8.5 along with this PR: #9651

CalvinNeo added 3 commits November 1, 2024 15:12

cp

04e3c8d

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

update client-c

6ab48a1

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

remove origin

4b886ce

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 1, 2024

remove .orig

0550bb8

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 1, 2024

format

a50e086

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 1, 2024

Merge branch 'master' into adapt-to-master

6dbb2fb

CalvinNeo requested review from JaySon-Huang, JinheLin and Lloyd-Pottiger November 1, 2024 10:46

JinheLin reviewed Nov 4, 2024

View reviewed changes

Lloyd-Pottiger approved these changes Nov 4, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 4, 2024

JinheLin reviewed Nov 4, 2024

View reviewed changes

CalvinNeo added 3 commits November 4, 2024 16:40

switch to context

753d199

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

Merge branch 'adapt-to-master' of github.com:CalvinNeo/tiflash into a…

3a981ee

…dapt-to-master

remove this settings

d89f44b

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

ti-chi-bot bot added the approved label Nov 4, 2024

CalvinNeo added 4 commits November 4, 2024 16:56

fix

b2c7d3b

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

fix

d90dc87

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

fix

f55fc28

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

lint

9744097

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

JaySon-Huang reviewed Nov 5, 2024

View reviewed changes

dbms/src/Interpreters/Context.h Outdated Show resolved Hide resolved

move all to shared

2e38be6

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

JaySon-Huang reviewed Nov 5, 2024

View reviewed changes

dbms/src/Interpreters/Context.cpp Outdated Show resolved Hide resolved

dbms/src/Interpreters/Context.cpp Outdated Show resolved Hide resolved

dbms/src/Interpreters/Context.cpp Outdated Show resolved Hide resolved

CalvinNeo and others added 5 commits November 6, 2024 13:31

Update dbms/src/Interpreters/Context.cpp

dc9dc56

Co-authored-by: JaySon <tshent@qq.com>

Update dbms/src/Interpreters/Context.cpp

3431646

Co-authored-by: JaySon <tshent@qq.com>

address cmt

fc328c1

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

Merge branch 'adapt-to-master' of github.com:CalvinNeo/tiflash into a…

bfd562c

…dapt-to-master

format

ce05560

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

CalvinNeo requested a review from JaySon-Huang November 6, 2024 05:56

JaySon-Huang approved these changes Nov 6, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 6, 2024

Merge branch 'master' into adapt-to-master

aa44b2c

ti-chi-bot bot merged commit bc0fc13 into pingcap:master Nov 7, 2024

Conversation

CalvinNeo commented Nov 1, 2024

What is changed and how it works?

Check List

Release note

Uh oh!

JinheLin Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

Uh oh!

JinheLin Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

CalvinNeo Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

JinheLin Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

CalvinNeo Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

JinheLin Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

CalvinNeo Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JaySon-Huang left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Nov 6, 2024

Uh oh!

ti-chi-bot bot commented Nov 6, 2024

[LGTM Timeline notifier]

Uh oh!

CalvinNeo commented Nov 6, 2024

Uh oh!

CalvinNeo commented Nov 6, 2024

Uh oh!

CalvinNeo commented Nov 7, 2024

Uh oh!

JaySon-Huang commented Nov 7, 2024

Uh oh!

JaySon-Huang commented Nov 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants