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

[opt](cloud) Make get tablet stats and update delete bitmap update lock be able to be in different fdb txns #45206

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

[opt](cloud) Make get tablet stats and update delete bitmap update lock be able to be in different fdb txns #45206

wants to merge 13 commits into from
+360 −36

Conversation

bobhan1
Copy link
Contributor

@bobhan1 bobhan1 commented Dec 9, 2024 •
edited
Loading

What problem does this PR solve?

#37670 let the meta service return the tablet compaction stats along with the getDeleteBitmapUpdateLockResponse to FE to let the BE know whether it should sync_rowsets() due to successful compaction on other BEs on the same tablet. That PR makes the process of reading tablets' stats and writing the delete bitmap update lock KV in one fdb txn to achieve the atomic sematic. However, when a load involves a large number of tablets, the process of reading tablets' stats may take longer than fdb txn's 5 seconds limitation and cause TXN_TOO_OLD error.
This PR re-arrange the process so that the read of tablet stats can be not necessarily in the same fdb txn with the txn which update the lock_info.lock_id. In detail, we do as the following:

  1. gain the delete bitmap update lock in MS (write delete bitmap update lock KV)
  2. read tablets' stats to get the compaction counts.
  3. check if the delete bitmap update lock is still held by the current load.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

  • Behavior changed:

    • No.
    • Yes.

  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@doris-robot
Copy link

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@bobhan1 bobhan1 force-pushed the fix-ms-get-delete-bitmap-lock-get-stats branch from 86d4945 to 0df6084 Compare December 10, 2024 04:03
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

github-actions[bot]
github-actions bot reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/test/meta_service_test.cpp Outdated Show resolved Hide resolved
@bobhan1 bobhan1 force-pushed the fix-ms-get-delete-bitmap-lock-get-stats branch from f74de71 to 54905d9 Compare December 10, 2024 05:18
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

github-actions[bot]
github-actions bot reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/test/meta_service_test.cpp Show resolved Hide resolved
cloud/test/meta_service_test.cpp Show resolved Hide resolved
github-actions[bot]
github-actions bot reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/test/meta_service_test.cpp Show resolved Hide resolved
github-actions[bot]
github-actions bot reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/test/meta_service_test.cpp Show resolved Hide resolved
cloud/test/meta_service_test.cpp Show resolved Hide resolved
@bobhan1 bobhan1 force-pushed the fix-ms-get-delete-bitmap-lock-get-stats branch 2 times, most recently from ae7e0df to fc8d9fe Compare December 10, 2024 06:54
github-actions[bot]
github-actions bot reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/test/meta_service_test.cpp Show resolved Hide resolved
cloud/test/meta_service_test.cpp Show resolved Hide resolved
github-actions[bot]
github-actions bot reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/test/meta_service_test.cpp Show resolved Hide resolved
cloud/test/meta_service_test.cpp Show resolved Hide resolved
github-actions[bot]
github-actions bot reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/test/meta_service_test.cpp Show resolved Hide resolved
@bobhan1
Copy link
Contributor Author

bobhan1 commented Dec 10, 2024

run buildall

zhannngchen
zhannngchen previously approved these changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@zhannngchen zhannngchen 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 Dec 11, 2024
@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions GitHub Actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@bobhan1 bobhan1 force-pushed the fix-ms-get-delete-bitmap-lock-get-stats branch from 379f2f2 to 2097edc Compare December 13, 2024 06:54
@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label Dec 13, 2024
github-actions[bot]
github-actions bot reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/src/meta-service/meta_service.cpp Show resolved Hide resolved
github-actions[bot]
github-actions bot reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/test/meta_service_test.cpp Show resolved Hide resolved
github-actions[bot]
github-actions bot reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

cloud/test/meta_service_test.cpp Show resolved Hide resolved
cloud/test/meta_service_test.cpp Show resolved Hide resolved
@bobhan1 bobhan1 requested a review from gavinchou December 13, 2024 08:34
@bobhan1
Copy link
Contributor Author

bobhan1 commented Dec 13, 2024

run buildall

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@zhannngchen zhannngchen zhannngchen left review comments

@gavinchou gavinchou Awaiting requested review from gavinchou

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
dev/3.0.x reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bobhan1 @doris-robot @gavinchou @zhannngchen