kvs-watch: support FLUX_KVS_STREAM flag #6523
Conversation
Problem: When a directory is watched with the WATCH_APPEND flag, an errno of EINVAL is returned. However, EISDIR is probably the more expected error when the invalid value is specifically a directory. Return EISDIR if there is an attempt to WATCH_APPEND specifically on a directory.
|
Any reason not to use the code written for the test to implement |
The two main reasons were A) output wise B) to ensure that you get so it ensures the streaming is working (i.e. vs a bug and I basically did a |
|
Maybe |
It wouldn't be hard to add to this PR, and I guess wouldn't be that bad of a "nice to have". It just wouldn't remove the need for the |
|
I suppose a |
Yeah that could be done, but seems a little excessive? |
|
Yeah totally up to you on that one :-) I was just throwing it out there. |
chu11
force-pushed
the
kvs_stream_flag
branch
from
b93fcdc to
23a9239
Compare
|
re-pushed w/
vs
was about 16 vs 11 seconds on the output from a I should probably add I noticed that |
|
Just watching a |
| @@ -35,7 +35,8 @@ enum kvs_op { | |||
| FLUX_KVS_APPEND = 32, | |||
There was a problem hiding this comment.
Commit message suggestion: restate the problem more directly, e.g.
Responses to KVS lookups must be contained within one message, no matter how large the value is.
src/modules/kvs-watch/kvs-watch.c
Outdated
| if (w->flags & FLUX_KVS_WATCH_APPEND) | ||
| errprintf (&err, "key watched with WATCH_APPEND shortened"); | ||
| else | ||
| errprintf (&err, "key being streamed shortened"); |
There was a problem hiding this comment.
Hmm, seems like in the STREAM case, we should just be working from a static version of the treeobj?
There was a problem hiding this comment.
oh yeah that's right. I was just trying to avoid the "WATCH_APPEND" in an error message.
There was a problem hiding this comment.
Maybe just make it
// cannot occur with FLUX_KVS_STREAM
errprintf (&err, "key watched with WATCH_APPEND shortened");if that makes sense? Otherwise the error message implies that it's an expected case which is a bit confusing.
src/modules/kvs-watch/kvs-watch.c
Outdated
| @@ -46,6 +46,7 @@ struct watcher { | |||
| bool index_valid; // flag if prev_start_index/prev_end_index set | |||
| int prev_start_index; // previous start index loaded | |||
| int prev_end_index; // previous end index loaded | |||
| int append_send_count; // number of indexes loaded (for FLUX_KVS_STREAM) | |||
There was a problem hiding this comment.
Probably this shouldn't be named "append" anything since there is no appending going on with FLUX_KVS_STREAM. Should it be a loaded_blob_count instead?
Plural of index is indices.
src/modules/kvs-watch/kvs-watch.c
Outdated
| errprintf (&err, | ||
| "value of key streamed was overwritten"); |
If i'm reading this correctly one goes back early? Probably the stdout header. But yeah, you'd think they should be more spread out. I'm wondering if the sqlite back end batches some lookups together and they all "return" at the same time? I should look into this a bit. |
|
That's one content response that comes back early but |
So I think I know the problem. Although I didn't profile it, I think the So when the The "mix" between content.load response and responses to kvs-lookup are more clear when data blob sizes are smaller. |
|
Ah that makes sense! Hmm, maybe we should add a raw option sometime :-) |
Yeah, this is definitely for study later on. I think there might be little performance improvements here and there we can do with that. The |
Problem: Responses to KVS lookups must be contained within one message, no matter how large the value is. Such a response can be slow and give the appearance that something is hanging. For example, a large amount of standard output stored in the KVS would not return immediately. Support a new FLUX_KVS_STREAM flag. It will use the mechanisms built into the kvs-watch module and the FLUX_KVS_WATCH_APPEND flag to stream each content blob of large values to the user. Once all the content is streamed, it will return ENODATA to indicate the stream is complete.
Problem: There is no coverage for the new FLUX_KVS_STREAM flag. Add coverage in t/t1007-kvs-lookup-watch.t and add new testing tool t/kvs/watch_stream.c.
Problem: The new FLUX_KVS_STREAM flag is not documented. Add documentation in flux_kvs_lookup(3).
Problem: The newly supported FLUX_KVS_STREAM flag does not have an equivalent option for support in flux-kvs. Support the --stream option under flux kvs get and flux kvs eventlog get.
Problem: The new flux kvs --stream option with flux kvs get and flux kvs eventlog get is not documented. Add documentation in flux-kvs(1).
Problem: There is no coverage of the new flux kvs --stream option with flux kvs get and flux kvs eventlog get. Add coverage in t1007-kvs-lookup-watch.t.
chu11
force-pushed
the
kvs_stream_flag
branch
from
23a9239 to
05aa903
Compare
|
re-pushed w/ fixes to comments above + added @garlick perhaps wanna take another quick skim just in case? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6523 +/- ##
==========================================
+ Coverage 83.61% 83.62% +0.01%
==========================================
Files 522 522
Lines 87734 87805 +71
==========================================
+ Hits 73356 73431 +75
+ Misses 14378 14374 -4
|
Problem: It would be convenient if there was a way to stream large data from the KVS by their individual content blobs, rather than getting a very large piece of content in one response. Such a response can be slow and give the appearance that something is hanging. For example, this ability would be useful for a large amount of standard output stored in the KVS.
Support a new FLUX_KVS_STREAM flag. It will use the mechanisms built into the kvs-watch module and the FLUX_KVS_WATCH_APPEND flag to stream each content blob to the user. Once all the content is streamed, it will return ENODATA to indicate the stream is complete.