-
Notifications
You must be signed in to change notification settings - Fork 51
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
libflux: support setting prep watcher priority #6367
Conversation
Problem: Watcher priorities are only configurable in check watchers, but would be useful in prepare watchers as well. Add support for prepare watcher priorities. Add unit tests.
The prep watchers are always called last, just before event loop might block in poll(2). I think this would only increase the priority of one prep watcher over another? Is that what you want? |
For my work in #6353 I want the "initial credits" to be sent to the client first before anything else happens. I suppose what you're saying is that because all prepare watchers are called before blocking, setting a priority for the prepare watcher doesn't matter? I suppose that makes sense. I guess I figured "something" could happen in other prepare watchers, so we'd want the "initial credits" prepare watcher to send initial credits before anything else might happen. |
Doesn't seem likely to have much effect? Any regular watcher that is ready would run before the prep watchers, and for the most part, work is not done in prep watchers. For example in the future implementation, continuations are called from check watchers. What about just having the subprocess server ignore the first credit callback from each stream and instead use info it already has to immediately generate one initial |
Oh maybe getopt works to get the buffer sizes already? |
I'm loosely already doing this. I think there may be a subtlety lost in this discussion. Lemme explain PR #6353 at a high level regarding the use of the prepare watcher. For local subprocesses, we basically fork/exec and return a For remote subprocesses, I basically do the same thing except I aggregate all of the channel credits into a single RPC back. By setting the prepare watcher priority high, I should be able to return the initial credits, even before the client gets notified that the remote subprocess has started (state == RUNNING, server sends "started" message). Thinking about it more (maybe this was your point in this discussion) I could simply just respond with an So the question is ... does setting the prepare watcher priority matter? Maybe for most cases, it doesn't. But it would be good to do it to be on the safe side? |
Got it. I guess my two points were:
|
Ok, if we feel the odds is it doesn't buy us much, then we could drop this PR.
Certainly is doable, which is related to the "NO_INITIAL_CREDITS" flag I mentioned in another thread. But I think having initial credits sent is important for the "api". We can discuss this in another PR thread though. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6367 +/- ##
==========================================
- Coverage 83.28% 83.28% -0.01%
==========================================
Files 524 524
Lines 86230 86232 +2
==========================================
- Hits 71820 71816 -4
- Misses 14410 14416 +6
|
Problem: Watcher priorities are only configurable in check watchers, but would be useful in prepare watchers as well.
Add support for prepare watcher priorities. Add unit tests.
split out from #6353. the "initial credits" from a subprocess should be sent immediately after the subprocess is returned to the caller. So we want to make the "initial credits" prepare watcher the first thing to be called.