Server: Introduce proxy state machine #9856

CalvinNeo · 2025-02-08T11:55:33Z

What problem does this PR solve?

Issue Number: close #9857 ref #9855

Problem Summary:

This is a trivial refinement of code to make proxy codes not so tangled with in Server::main.

This endeavor is to start TiFlash storage layer alone without Proxy and KVStore module, and make further cp easy.

We'd like to cp this PR into release-8.5 in order to keep compatible with cse TiFlash.

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 <[email protected]>

CalvinNeo · 2025-02-08T12:39:26Z

/cc @JaySon-Huang @Lloyd-Pottiger @JinheLin

Signed-off-by: Calvin Neo <[email protected]>

CalvinNeo · 2025-02-08T12:58:21Z

/check-issue-triage

CalvinNeo · 2025-02-08T12:59:41Z

/check-issue-triage-complete

dbms/src/Server/Server.cpp

Signed-off-by: Calvin Neo <[email protected]>

Co-authored-by: Lloyd-Pottiger <[email protected]>

Signed-off-by: Calvin Neo <[email protected]>

dbms/src/Storages/KVStore/ProxyStateMachine.h

dbms/src/Server/Server.cpp

Signed-off-by: Calvin Neo <[email protected]>

JaySon-Huang · 2025-02-10T06:55:19Z

And we can Move function getKeyManager to ProxyStateMachine to simplify the code in Server.cpp. Checkout this PR please CalvinNeo#14

Signed-off-by: Calvin Neo <[email protected]>

CalvinNeo · 2025-02-10T07:25:56Z

getKeyManager

And we can Move function getKeyManager to ProxyStateMachine to simplify the code in Server.cpp. Checkout this PR please CalvinNeo#14

It seems to me that the creation of the KeyManager is not strongly related to the proxy's lifetime. Here are the reasons:

The ProxyStateMachine is a state machine that manages the lifetime of Proxy and KVStore, so we can just call corresponding methods of this state machine when we want to bootstrap/restore/run/stop/destroy/join proxy. It is not a wrapper of ProxyHelper and KVStore's functionality.
KeyManager is neither part of Proxy nor KVStore. It just relies on checkEncryptionEnabled and ProxyHelper(The dependency of EngineStoreServerWrap is not necessary, could be addressed later). Both of these are the functionalities that proxy provides, rather than what proxy relies or what could affect how proxy would bootstrap or be destroyed.

So, instead, I think this could be extracted into a function like setOpenFileLimit. Or even more we can create a state machine for the storage layer if it is necessary. Neither cases IMO show enough importance that we should address in this PR, since these changes seem not necessary to be cherry-picked into release-8.5.

JaySon-Huang · 2025-02-10T07:35:21Z

If Encryption-at-rest is enable, then DataKeyManager rely on the ProxyHelper lifetime to get the encryption key. So I think it should belong to ProxyStateMachine

CalvinNeo · 2025-02-10T07:43:23Z

If Encryption-at-rest is enable, then DataKeyManager rely on the ProxyHelper lifetime to get the encryption key. So I think it should belong to ProxyStateMachine

Yes it does, and so do TMTContext, KVStore and Context. The lifetime and functionality of Proxy can affect all these modules. Currently some modules related to each other in a complicated pattern.

So although the ProxyStateMachine is designed to manage the lifetime of Proxy, it is really Server::main that drives the state machine thus eventually controlling the lifetime of Proxy(/KVStore/TMTContext/...). In fact, it controls all lifetimes, like Context, TMTContext and so on. The ProxyStateMachine manages the lifetime of the Proxy, by exposing methods that should be correctly executed at different timepoints.

Moving this utilities into a ProxyStateMachine, so:

Server::main is responsible for calling those methods at the right time.
ProxyStateMachine is responsible for implementing those methods in a right way.

Signed-off-by: Calvin Neo <[email protected]>

dbms/src/Server/Server.cpp

Co-authored-by: jinhelin <[email protected]>

dbms/src/Server/Server.cpp

Co-authored-by: jinhelin <[email protected]>

JinheLin · 2025-02-10T08:24:17Z

dbms/src/Storages/KVStore/ProxyStateMachine.h

+        is_proxy_runnable = tryParseFromConfig(config, disaggregated_mode, use_autoscaler, log);
+
+        args.push_back("TiFlash Proxy");
+        for (const auto & v : val_map)


We use val_map to update args here, but val_map and args will be update simultaneously in addExtraArgs. It looks a bit chaotic.

Why need val_map in TiFlashProxyConfig?

Oh...args contains raw pointer to val_map.

I think val_map is for ownership, and args is for view

I have refined the code of args in this PR: CalvinNeo#15

Only maintain val_map and generate args by calling getArgs to avoid maintains val_map and args simultaneously.

Yes, it is clearer, thx. Merged.

JaySon-Huang · 2025-02-10T08:55:18Z

dbms/src/Storages/KVStore/ProxyStateMachine.h

+        else
+        {
+            // Could be a auto-scaled compute node.
+            LOG_INFO(log, "KVStore is not initialized because no store_ident is provided");
+        }


I think we can remove this branch. Now for non-disagg mode, the UniPS is not enabled so the store_ident is nullopt.

And for non-disagg mode, the StoreIdnet will be set by the FFI fn_set_store. Outputing this logging could be misleading.

Signed-off-by: Calvin Neo <[email protected]>

JaySon-Huang

LGTM

ti-chi-bot · 2025-02-10T14:44:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JaySon-Huang,JinheLin]

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 · 2025-02-10T14:44:10Z

[LGTM Timeline notifier]

Timeline:

2025-02-10 09:34:01.041150183 +0000 UTC m=+262683.437372230: ☑️ agreed by JaySon-Huang.
2025-02-10 14:44:08.941860844 +0000 UTC m=+281291.338082905: ☑️ agreed by JinheLin.

CalvinNeo · 2025-02-11T03:22:03Z

/cherry-pick release-8.5

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2025-02-11T03:22:41Z

@CalvinNeo: new pull request created to branch release-8.5: #9861.
But this PR has conflicts, please resolve them!

In response to this:

/cherry-pick release-8.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ref pingcap#9855, close pingcap#9857 Signed-off-by: Calvin Neo <[email protected]> Co-authored-by: jinhelin <[email protected]>

add proxy state machine

af96bf6

Signed-off-by: Calvin Neo <[email protected]>

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 Feb 8, 2025

format

46618c5

Signed-off-by: Calvin Neo <[email protected]>

ti-chi-bot bot requested review from JaySon-Huang, JinheLin and Lloyd-Pottiger February 8, 2025 12:39

CalvinNeo added 2 commits February 8, 2025 20:40

reformat

68a9049

Signed-off-by: Calvin Neo <[email protected]>

move some out

1ae07e5

Signed-off-by: Calvin Neo <[email protected]>

ti-chi-bot bot added the do-not-merge/needs-triage-completed label Feb 8, 2025

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Feb 8, 2025

Lloyd-Pottiger reviewed Feb 10, 2025

View reviewed changes

dbms/src/Server/Server.cpp Outdated Show resolved Hide resolved

CalvinNeo and others added 5 commits February 10, 2025 11:04

some comments

158fbef

Signed-off-by: Calvin Neo <[email protected]>

Update dbms/src/Server/Server.cpp

09b72b5

Co-authored-by: Lloyd-Pottiger <[email protected]>

fix compile

2d08e4a

Signed-off-by: Calvin Neo <[email protected]>

rename

19698cb

Signed-off-by: Calvin Neo <[email protected]>

Merge branch 'master' into proxy-sm

2528d0a

CalvinNeo requested a review from Lloyd-Pottiger February 10, 2025 03:45

JaySon-Huang reviewed Feb 10, 2025

View reviewed changes

CalvinNeo added 7 commits February 10, 2025 12:04

address comments

ea67259

Signed-off-by: Calvin Neo <[email protected]>

Merge branch 'proxy-sm' of github.com:CalvinNeo/tiflash into proxy-sm

9c35655

rename

6f0110b

Signed-off-by: Calvin Neo <[email protected]>

set

7a2e8c0

Signed-off-by: Calvin Neo <[email protected]>

use restoreKVStore

df5e837

Signed-off-by: Calvin Neo <[email protected]>

fix compile

9fcc312

Signed-off-by: Calvin Neo <[email protected]>

fix compile 2

ae7de96

Signed-off-by: Calvin Neo <[email protected]>

CalvinNeo requested a review from JaySon-Huang February 10, 2025 06:01

add comments

eaf1a61

Signed-off-by: Calvin Neo <[email protected]>

pick others

c6a18bf

Signed-off-by: Calvin Neo <[email protected]>

JinheLin reviewed Feb 10, 2025

View reviewed changes

dbms/src/Server/Server.cpp Outdated Show resolved Hide resolved

Update dbms/src/Server/Server.cpp

4382af9

Co-authored-by: jinhelin <[email protected]>

JinheLin reviewed Feb 10, 2025

View reviewed changes

dbms/src/Server/Server.cpp Outdated Show resolved Hide resolved

Update dbms/src/Server/Server.cpp

f206c99

Co-authored-by: jinhelin <[email protected]>

JinheLin reviewed Feb 10, 2025

View reviewed changes

JaySon-Huang reviewed Feb 10, 2025

View reviewed changes

CalvinNeo and others added 3 commits February 10, 2025 16:59

fix

5e186f0

Signed-off-by: Calvin Neo <[email protected]>

Merge branch 'proxy-sm' of github.com:CalvinNeo/tiflash into proxy-sm

0551f12

refine args (#15)

60683ed

JaySon-Huang approved these changes Feb 10, 2025

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Feb 10, 2025

CalvinNeo requested a review from JinheLin February 10, 2025 09:35

JinheLin approved these changes Feb 10, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 10, 2025

ti-chi-bot bot merged commit 0001db3 into pingcap:master Feb 10, 2025
5 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Feb 11, 2025

This is an automated cherry-pick of pingcap#9856

e63aa7c

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Feb 11, 2025

Server: Introduce proxy state machine (#9856) #9861

Closed

12 tasks

JaySon-Huang pushed a commit to CalvinNeo/tiflash that referenced this pull request Feb 11, 2025

Server: Introduce proxy state machine (pingcap#9856)

eddb379

ref pingcap#9855, close pingcap#9857 Signed-off-by: Calvin Neo <[email protected]> Co-authored-by: jinhelin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server: Introduce proxy state machine #9856

Server: Introduce proxy state machine #9856

CalvinNeo commented Feb 8, 2025 •

edited

Loading

CalvinNeo commented Feb 8, 2025

CalvinNeo commented Feb 8, 2025

CalvinNeo commented Feb 8, 2025

JaySon-Huang commented Feb 10, 2025

CalvinNeo commented Feb 10, 2025 •

edited

Loading

JaySon-Huang commented Feb 10, 2025

CalvinNeo commented Feb 10, 2025 •

edited

Loading

JinheLin Feb 10, 2025 •

edited

Loading

JinheLin Feb 10, 2025

CalvinNeo Feb 10, 2025

JinheLin Feb 10, 2025 •

edited

Loading

CalvinNeo Feb 10, 2025

JaySon-Huang Feb 10, 2025

CalvinNeo Feb 10, 2025

JaySon-Huang left a comment

ti-chi-bot bot commented Feb 10, 2025

ti-chi-bot bot commented Feb 10, 2025

CalvinNeo commented Feb 11, 2025

ti-chi-bot commented Feb 11, 2025

Server: Introduce proxy state machine #9856

Server: Introduce proxy state machine #9856

Conversation

CalvinNeo commented Feb 8, 2025 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

CalvinNeo commented Feb 8, 2025

CalvinNeo commented Feb 8, 2025

CalvinNeo commented Feb 8, 2025

JaySon-Huang commented Feb 10, 2025

CalvinNeo commented Feb 10, 2025 • edited Loading

JaySon-Huang commented Feb 10, 2025

CalvinNeo commented Feb 10, 2025 • edited Loading

JinheLin Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

JinheLin Feb 10, 2025

Choose a reason for hiding this comment

CalvinNeo Feb 10, 2025

Choose a reason for hiding this comment

JinheLin Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

CalvinNeo Feb 10, 2025

Choose a reason for hiding this comment

JaySon-Huang Feb 10, 2025

Choose a reason for hiding this comment

CalvinNeo Feb 10, 2025

Choose a reason for hiding this comment

JaySon-Huang left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Feb 10, 2025

ti-chi-bot bot commented Feb 10, 2025

[LGTM Timeline notifier]

CalvinNeo commented Feb 11, 2025

ti-chi-bot commented Feb 11, 2025

CalvinNeo commented Feb 8, 2025 •

edited

Loading

CalvinNeo commented Feb 10, 2025 •

edited

Loading

CalvinNeo commented Feb 10, 2025 •

edited

Loading

JinheLin Feb 10, 2025 •

edited

Loading

JinheLin Feb 10, 2025 •

edited

Loading