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

fix: thread-safe for RequestAggUnionRunner #1838

Merged
merged 1 commit into from
May 19, 2022

Conversation

zhanghaohit
Copy link
Collaborator

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    close long window processing core dump under stress test #1837

  • What is the current behavior? (You can also link to an open issue here)
    Currently aggregator_ is re-used for every RequestAggUnionRunner::Run, which is not thread-safe if the ClusterJob is also re-used and run in multiple threads (e.g., stored procedure/deployment request).

  • What is the new behavior (if this is a feature change)?
    aggregator is dynamically created for every Run

@zhanghaohit zhanghaohit self-assigned this May 18, 2022
@github-actions github-actions bot added execute-engine hybridse sql engine storage-engine openmldb storage engine. nameserver & tablet labels May 18, 2022
@zhanghaohit zhanghaohit added the bug Something isn't working label May 18, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2022

SDK Test Report

  74 files    74 suites   8m 2s ⏱️
175 tests 172 ✔️ 3 💤 0
216 runs  213 ✔️ 3 💤 0

Results for commit 0806c05.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

HybridSE Linux Test Report

       67 files       235 suites   5m 50s ⏱️
19 059 tests 19 057 ✔️ 2 💤 0

Results for commit 0806c05.

@github-actions
Copy link
Contributor

HybridSE Mac Test Report

       67 files       235 suites   6m 23s ⏱️
19 059 tests 19 057 ✔️ 2 💤 0

Results for commit 0806c05.

@github-actions
Copy link
Contributor

Linux Test Report

       57 files       186 suites   1h 6m 11s ⏱️
  8 543 tests   8 535 ✔️ 8 💤 0
12 529 runs  12 521 ✔️ 8 💤 0

Results for commit 0806c05.

@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #1838 (0806c05) into main (b8ff05e) will decrease coverage by 0.01%.
The diff coverage is 95.77%.

@@             Coverage Diff              @@
##               main    #1838      +/-   ##
============================================
- Coverage     73.74%   73.73%   -0.02%     
  Complexity      347      347              
============================================
  Files           614      614              
  Lines        120314   120364      +50     
  Branches       1015     1015              
============================================
+ Hits          88731    88753      +22     
- Misses        31374    31402      +28     
  Partials        209      209              
Impacted Files Coverage Δ
hybridse/src/vm/runner.h 92.84% <ø> (ø)
hybridse/src/vm/runner.cc 67.77% <88.88%> (-0.04%) ⬇️
src/catalog/tablet_catalog_test.cc 96.26% <98.11%> (+0.11%) ⬆️
...s/query_response_time/deploy_query_response_time.h 81.25% <0.00%> (-18.75%) ⬇️
...atistics/query_response_time/query_response_time.h 78.26% <0.00%> (-4.35%) ⬇️
.../query_response_time/deploy_query_response_time.cc 97.10% <0.00%> (-2.90%) ⬇️
src/sdk/db_sdk.cc 61.67% <0.00%> (-0.46%) ⬇️
src/client/tablet_client.cc 49.63% <0.00%> (-0.21%) ⬇️
src/tablet/tablet_impl.cc 58.96% <0.00%> (-0.18%) ⬇️
src/sdk/sql_cluster_router.cc 54.84% <0.00%> (-0.13%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8ff05e...0806c05. Read the comment docs.

Copy link
Collaborator

@tobegit3hub tobegit3hub left a comment

Choose a reason for hiding this comment

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

LGTM

@zhanghaohit zhanghaohit merged commit 695416f into 4paradigm:main May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execute-engine hybridse sql engine storage-engine openmldb storage engine. nameserver & tablet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

long window processing core dump under stress test
4 participants