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.

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

[refactor](schema change) Using tablet schema shared ptr instead of raw ptr #11475

Merged
merged 27 commits into from
Aug 5, 2022

Conversation

yiguolei
Copy link
Contributor

@yiguolei yiguolei commented Aug 3, 2022

Proposed changes

This is part of light weight schema change #10136

Problem summary

Describe your changes.

Checklist(Required)

  1. Does it affect the original behavior:
    • Yes
    • No
    • I don't know
  2. Has unit tests been added:
    • Yes
    • No
    • No Need
  3. Has document been added or modified:
    • Yes
    • No
    • No Need
  4. Does it need to update dependencies:
    • Yes
    • No
  5. Are there any changes that cannot be rolled back:
    • Yes (If Yes, please explain WHY)
    • No

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@wangbo
Copy link
Contributor

wangbo commented Aug 4, 2022

Have you done a stress test to see whether this is a performance drop for this pr?
Including query and insert.

@yiguolei
Copy link
Contributor Author

yiguolei commented Aug 4, 2022

Have you done a stress test to see whether this is a performance drop for this pr? Including query and insert.

No, because tablet schema is only used in control path, it is not on data path. And there will be a nightly performance benchmark in selectdb. If we find performance drops we could revert this PR.

@Lchangliang
Copy link
Contributor

LGTM

@yiguolei
Copy link
Contributor Author

yiguolei commented Aug 4, 2022

@wangbo I have done a simple performance test.

  1. set scanner thread to 48.
  2. set page cache = true and cache all data into page cache.
  3. run 3 concurrenty queries.

compare current branch with a47eff1 commit.
both of them the avg latency is 590-595ms.
And the cpu is about 3000%+.
I think the shared ptr does not affect performance.

Copy link
Contributor

@wangbo wangbo left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Aug 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

PR approved by anyone and no changes requested.

@yiguolei yiguolei merged commit 321107c into apache:master Aug 5, 2022
@yiguolei yiguolei deleted the using_tabletsptr branch March 30, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. area/vectorization reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants