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 races in aggregator if there are concurrent Puts #2472

Merged
merged 5 commits into from
Sep 15, 2022

Conversation

zhanghaohit
Copy link
Collaborator

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Fix Failed to import data with longwindow in online sever #2465

  • What is the current behavior? (You can also link to an open issue here)
    The binlog_offset may be out-of-order to the aggregator if there are concurrent Puts.
    However aggregator assumes it is strictly in order.

  • What is the new behavior (if this is a feature change)?
    Aggregator update is protected by binlog mutex and is called as a closure parameter inside binlog.

This is a quick fix. A better solution will be doing asynchronous update to the aggregators from the binlog.

@zhanghaohit zhanghaohit requested a review from nautaa September 14, 2022 06:04
@github-actions github-actions bot added the storage-engine openmldb storage engine. nameserver & tablet label Sep 14, 2022
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Base: 35.89% // Head: 75.82% // Increases project coverage by +39.93% 🎉

Coverage data is based on head (ee0780f) compared to base (5ac4667).
Patch coverage: 97.14% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##               main    #2472       +/-   ##
=============================================
+ Coverage     35.89%   75.82%   +39.93%     
- Complexity      368      371        +3     
=============================================
  Files           154      638      +484     
  Lines          9007   120084   +111077     
  Branches       1056     1058        +2     
=============================================
+ Hits           3233    91053    +87820     
- Misses         5558    28814    +23256     
- Partials        216      217        +1     
Impacted Files Coverage Δ
src/replica/log_replicator.h 100.00% <ø> (ø)
src/storage/aggregator.cc 78.81% <50.00%> (ø)
src/tablet/tablet_impl.cc 61.68% <70.00%> (ø)
src/replica/log_replicator.cc 81.99% <100.00%> (ø)
src/tablet/tablet_impl.h 100.00% <100.00%> (ø)
src/tablet/tablet_impl_test.cc 99.54% <100.00%> (ø)
python/openmldb_sdk/test/case_conf.py
python/openmldb_sdk/test/sql_magic_test.py
python/openmldb_sdk/test/dbapi_test.py
python/openmldb_sdk/test/sdk_smoke_test.py
... and 495 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2022

Linux Test Report

       55 files       203 suites   1h 4m 11s ⏱️
  9 904 tests   9 897 ✔️ 7 💤 0
14 580 runs  14 573 ✔️ 7 💤 0

Results for commit ee0780f.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2022

SDK Test Report

  81 files    81 suites   8m 2s ⏱️
168 tests 166 ✔️ 2 💤 0
210 runs  208 ✔️ 2 💤 0

Results for commit ee0780f.

♻️ This comment has been updated with latest results.

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 736bd1d into 4paradigm:main Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
storage-engine openmldb storage engine. nameserver & tablet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to import data with longwindow in online sever
4 participants