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

Dirty Memory: Revert allow_stall naming from allow_delays_and_stalls #549

Merged
merged 6 commits into from
Jun 12, 2023

Conversation

Yuval-Ariel
Copy link
Contributor

@Yuval-Ariel Yuval-Ariel commented Jun 11, 2023

During #423 , the WriteBufferManager ctor parameter allow_stall was renamed to allow_delays_and_stalls to reflect its new capabilities (delaying in addition to stalling (stopping) writes). This creates confusion and unwanted behavior so this PR reverts this change and the new functionality of #423 is controller via the old WriteBufferManager ctor parameter named allow_stall.

this PR also sets back the allow_stall default value to false to avoid changing current expected behavior.

@Yuval-Ariel Yuval-Ariel requested a review from udi-speedb June 11, 2023 11:21
@Yuval-Ariel Yuval-Ariel self-assigned this Jun 11, 2023
@udi-speedb
Copy link
Contributor

@Yuval-Ariel - Since this is not part of the release, please add the continuous gradual delay part we have discussed and I will review when that is done. Thanks

@udi-speedb
Copy link
Contributor

@Yuval-Ariel I have partially reviewed.

memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
HISTORY.md Outdated Show resolved Hide resolved
@Yuval-Ariel Yuval-Ariel requested a review from udi-speedb June 12, 2023 06:43
@Yuval-Ariel
Copy link
Contributor Author

@udi-speedb it has to be part of the release since its an integral part of #423 . so #552 will be handled elsewhere. plz review asap

@Yuval-Ariel
Copy link
Contributor Author

we've agreed that for this PR, the allow_delays_and_stalls will be reverted to allow_stall and will be used solely to control the functionality of delaying writes with the WBM. The separation to two different flags, allow_stall and allow_delay is the best approach so that users will have the most flexibility and the behavior can remain unchanged but will be handled in #552 .
This is done in order to keep the documentation organized and not ship out an unfinished feature.

add allow_delays as an additional flag to the WBM ctor to enable the
connection between the WBM and Global WriteController.
set allow_delays = true to enable memory limit delays.
and set stop delay to be the last delay step instead of kMinWriteRate
@Yuval-Ariel Yuval-Ariel force-pushed the revert_wbm_allow_stalls_name branch from cee25d5 to 86a3d6c Compare June 12, 2023 19:06
@Yuval-Ariel Yuval-Ariel merged commit 72d4181 into main Jun 12, 2023
@Yuval-Ariel Yuval-Ariel deleted the revert_wbm_allow_stalls_name branch June 12, 2023 19:10
udi-speedb pushed a commit that referenced this pull request Nov 22, 2023
…549)

During #423 , the WriteBufferManager ctor parameter allow_stall was renamed to allow_delays_and_stalls to reflect its new capabilities (delaying in addition to stalling (stopping) writes). This creates confusion and unwanted behavior so this PR reverts this change and the new functionality of #423 is controller via the old WriteBufferManager ctor parameter named allow_stall.

This commit also sets back the allow_stall default value to false to avoid changing current expected behavior.
udi-speedb pushed a commit that referenced this pull request Dec 5, 2023
…549)

During #423 , the WriteBufferManager ctor parameter allow_stall was renamed to allow_delays_and_stalls to reflect its new capabilities (delaying in addition to stalling (stopping) writes). This creates confusion and unwanted behavior so this PR reverts this change and the new functionality of #423 is controller via the old WriteBufferManager ctor parameter named allow_stall.

This commit also sets back the allow_stall default value to false to avoid changing current expected behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants