-
Notifications
You must be signed in to change notification settings - Fork 377
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
feature: Transit buffer #3572
feature: Transit buffer #3572
Conversation
86f384d
to
24061de
Compare
Feel free to give some input on what the defaults should be. Zero bytes transit buffer has to be allowed, because that disables the feature. |
24061de
to
65c253a
Compare
I rebased against latest and added a commit message. The message is not very long right now so if there's anything you would like me to add there, I will be happy to do that. |
fd651bf
to
b420a0d
Compare
8c9d469
to
da03c94
Compare
I force-pushed a split of the transit buffer patch into 4 commits. Doing so, I tried to polish the commit log, C code and test cases. I couldn't make c112 fail reliably with transit buffer disabled (it passes reliably when enabled) so I didn't keep it. |
powwow: @nigoroll to propose an implementation without the timeout |
FTR, /me feels bad about not having got to this todo item |
rebased onto master here https://github.com/nigoroll/varnish-cache/tree/transit_buffering |
@fwsGonzo / @dridi could you look into this? |
I am running on an X1 Carbon laptop running the same test with lower parallelism, but I am unable to reproduce. Should add that I am extending varnishtest to support my CMake build system, but other than that everything is the same. I rebased using your transit_buffering branch. |
@fwsGonzo For our convenience, would you force-push this branch please? Regarding the test, there was no CMake/make involved in the test failure, just pure |
I pushed my VC working tree here: https://github.com/fwsGonzo/varnish-cache/tree/latest_transit There are a few scuffed commits in there as I am trying to keep the fuzzing support up to date, but it's getting harder with time. |
@fwsGonzo can we just have the branch for this PR updated without unrelated commits please? |
You can push to it if you want! |
da03c94
to
a2ca25f
Compare
This clarifies which length this field is about. Better diff with the --word-diff --word-diff-regex='\w+' options. Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
It pauses the fetch progress when clients are lagging behind for uncacheable streaming deliveries. Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
To override the global parameter on a per-fetch basis. Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
a2ca25f
to
7e978a2
Compare
So I would like to propose to remove the timeout while waiting on the condition variable, based on the argument that we do not need it if our code is correct, and if our code is not correct, we should rather fix it than add stop-gaps. |
Dear all, Do we have an estimation when it will be merged and for which version? Really interesting feature that can save memory and optimize stability 😁 |
Thank you but it's strange that it's not merged in this case 🤔 |
Pushed in the context of this PR: caf49d8 |
This PR implements the transit buffer feature (previously called pass buffering).