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: clear shared buffer on stop_all_actors #3513

Merged
merged 3 commits into from
Jul 1, 2022

Conversation

hzxa21
Copy link
Collaborator

@hzxa21 hzxa21 commented Jun 28, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

During the recovery process, we stop all actors in the compute node but we miss to clear buffers in storage, which keeps the memory unreleased. This PR fixes the problem by adding a method to clear shared buffer and calling it on stop_all_actors.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

@hzxa21 hzxa21 requested a review from zwang28 June 28, 2022 03:58
@github-actions github-actions bot added the type/fix Bug fix label Jun 28, 2022
@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #3513 (d4bfbd0) into main (d2ec610) will increase coverage by 0.03%.
The diff coverage is 58.55%.

❗ Current head d4bfbd0 differs from pull request most recent head 1d0940d. Consider uploading reports for the commit 1d0940d to get more accurate results

@@            Coverage Diff             @@
##             main    #3513      +/-   ##
==========================================
+ Coverage   74.41%   74.44%   +0.03%     
==========================================
  Files         771      770       -1     
  Lines      108925   108509     -416     
==========================================
- Hits        81052    80778     -274     
+ Misses      27873    27731     -142     
Flag Coverage Δ
rust 74.44% <58.55%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/compute/src/rpc/service/stream_service.rs 0.00% <0.00%> (ø)
src/storage/src/hummock/shared_buffer/mod.rs 90.58% <ø> (ø)
src/storage/src/hummock/state_store.rs 82.40% <0.00%> (-1.66%) ⬇️
src/storage/src/memory.rs 78.57% <0.00%> (-1.31%) ⬇️
src/storage/src/monitor/monitored_store.rs 1.81% <0.00%> (-0.07%) ⬇️
src/storage/src/panic_store.rs 0.00% <0.00%> (ø)
src/storage/src/store.rs 42.85% <0.00%> (-21.85%) ⬇️
src/stream/src/task/barrier_manager.rs 67.67% <ø> (ø)
...c/stream/src/task/barrier_manager/managed_state.rs 72.72% <ø> (-0.28%) ⬇️
src/stream/src/task/stream_manager.rs 0.00% <0.00%> (ø)
... and 104 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@hzxa21 hzxa21 requested a review from wenym1 June 29, 2022 06:10
@hzxa21 hzxa21 force-pushed the patrick/fix-stop-all-actors branch from fffbcf0 to fad15c8 Compare June 29, 2022 07:25
Copy link
Contributor

@wenym1 wenym1 left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

src/storage/src/hummock/local_version_manager.rs Outdated Show resolved Hide resolved
@hzxa21 hzxa21 added the mergify/can-merge Indicates that the PR can be added to the merge queue label Jul 1, 2022
@mergify
Copy link
Contributor

mergify bot commented Jul 1, 2022

Hey @hzxa21, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by commenting with @mergifyio requeue. More details can be found on the Queue: Embarked in merge train check-run.

@hzxa21 hzxa21 force-pushed the patrick/fix-stop-all-actors branch from 1d0940d to 56c8d3d Compare July 1, 2022 03:47
@mergify mergify bot merged commit 2430b63 into main Jul 1, 2022
@mergify mergify bot deleted the patrick/fix-stop-all-actors branch July 1, 2022 03:58
huangjw806 pushed a commit that referenced this pull request Jul 5, 2022
* fix: clear shared buffer on stop_all_actors

* Use a SharedBufferEvent to clear shared buffer and pending write requests

* address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergify/can-merge Indicates that the PR can be added to the merge queue type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants