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.

Sign up for GitHub

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

Jump to bottom

KVStore: Get addressed engine-addr from proxy #9174

Merged
merged 11 commits into from
Jul 3, 2024
Merged

KVStore: Get addressed engine-addr from proxy #9174

merged 11 commits into from
Jul 3, 2024

Conversation

CalvinNeo
Copy link
Member

@CalvinNeo CalvinNeo commented Jul 1, 2024 •
edited
Loading

What problem does this PR solve?

Issue Number: close #8382

Problem Summary:

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@CalvinNeo
a
1331f7f 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 1, 2024
CalvinNeo added 2 commits July 1, 2024 23:24
@CalvinNeo
aaa
7a753fd 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@CalvinNeo
aaa
a693d1f 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@ti-chi-bot ti-chi-bot bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/needs-linked-issue labels Jul 1, 2024
@CalvinNeo
a
95b387a 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@CalvinNeo
Copy link
Member Author

CalvinNeo commented Jul 1, 2024 •
edited
Loading

image
img_v3_02cc_bb025186-24b1-4388-9f1e-789b95b334fg
img_v3_02cc_db961c38-398f-4467-a767-301e14ead85g
image

@CalvinNeo
a
5afa3da 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 1, 2024
JaySon-Huang
JaySon-Huang reviewed Jul 2, 2024
View reviewed changes
dbms/src/Storages/KVStore/TMTContext.cpp
Comment on lines +173 to +174
if (kvstore->getProxyConfigSummay().valid)
{
Copy link
Contributor

@JaySon-Huang JaySon-Huang Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Seems the proxy config summary is only set as valid after KVStore::restore -> KVStore::fetchProxyConfig. That is done in TMTContext::restore, which is done after the TMTContext instance is build. Can we really get the proxy config to create the S3 GC Owner?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will check that.

CalvinNeo added 3 commits July 2, 2024 15:21
@CalvinNeo
a
cc29a3c 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@CalvinNeo
f
42fa9c3 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@CalvinNeo
f
3d796d4 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@CalvinNeo CalvinNeo requested a review from JaySon-Huang July 2, 2024 08:31
CalvinNeo added 2 commits July 2, 2024 18:23
@CalvinNeo
a
b6672de 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@CalvinNeo
a
6f6c3ef 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@CalvinNeo
a
23f5369 
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jul 3, 2024
JinheLin
JinheLin reviewed Jul 3, 2024
View reviewed changes
dbms/src/Storages/KVStore/TMTContext.cpp
&& !context.getSharedContextDisagg()->isDisaggregatedComputeMode())
{
s3gc_owner = OwnerManager::createS3GCOwner(context, /*id*/ raft_config.advertise_engine_addr, etcd_client);
if (kvstore->getProxyConfigSummay().valid)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the initialization sequence is a bit chaotic. Can we ensure that the proxy config summary is valid before creating the S3GCOwner?

Copy link
Member Author

@CalvinNeo CalvinNeo Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, fetchProxyConfig acquires "addressed" proxy config from proxy, which is already set by proxy in run_impl.

    let mut proxy = RaftStoreProxy::new(
        AtomicU8::new(RaftProxyStatus::Idle as u8),
        tikv.core.encryption_key_manager.clone(),
        Some(Box::new(ReadIndexClient::new(
            tikv.router.clone(),
            SysQuota::cpu_cores_quota() as usize * 2,
        ))),
        None,
        Some(tikv.pd_client.clone()),
        proxy_config_str,
    );

@CalvinNeo CalvinNeo requested a review from JinheLin July 3, 2024 03:11
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jul 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [JinheLin,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 ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 3, 2024
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jul 3, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-07-03 01:39:27.55948453 +0000 UTC m=+1374894.044973357: ☑️ agreed by Lloyd-Pottiger.
  • 2024-07-03 03:56:13.191817252 +0000 UTC m=+1383099.677306084: ☑️ agreed by JinheLin.

@ti-chi-bot ti-chi-bot bot merged commit 20d4616 into pingcap:master Jul 3, 2024
4 of 5 checks passed
JaySon-Huang pushed a commit to JaySon-Huang/tiflash that referenced this pull request Aug 2, 2024
@CalvinNeo
KVStore: Get addressed engine-addr from proxy (pingcap#9174) (pingcap…
157c6ed 
…#231)

Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lloyd-Pottiger Lloyd-Pottiger Lloyd-Pottiger approved these changes

@JinheLin JinheLin JinheLin approved these changes

@JaySon-Huang JaySon-Huang Awaiting requested review from JaySon-Huang

Assignees
No one assigned
Labels
approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce two stages FAP
4 participants
@CalvinNeo @JaySon-Huang @JinheLin @Lloyd-Pottiger