Impl subscribe subcommand & subscription smoketests #1343
Conversation
coolreader18
force-pushed
the
noa/subscribe-subcommand
branch
from
ab7bd8f to
fa755cb
Compare
coolreader18
force-pushed
the
noa/subscribe-subcommand
branch
from
fa755cb to
a134c4d
Compare
coolreader18
force-pushed
the
noa/subscribe-subcommand
branch
from
a134c4d to
f09b53d
Compare
| @@ -331,16 +345,21 @@ where | |||
| .get_or_launch_module_host(database, instance_id) | |||
| .await | |||
| .map_err(log_and_500)?; | |||
There was a problem hiding this comment.
From let address to here, this code is shared with fn describe. If we deduplicated, this would help towards a separate endpoint.
There was a problem hiding this comment.
We can do this in a follow up.
| "The timeout, in seconds, after which to disconnect and stop receiving \ | ||
| subscription messages. If `-n` is specified, it will stop after whichever | ||
| one comes first.", |
There was a problem hiding this comment.
This looks like a workable alternate semantics.
Note to self: change to this in the proposal.
| .insert(header::AUTHORIZATION, auth_header.try_into().unwrap()); | ||
| } | ||
| let (mut ws, _) = tokio_tungstenite::connect_async(req).await?; | ||
|
|
There was a problem hiding this comment.
How does this account for the request_id received matching the default we sent?
There was a problem hiding this comment.
We don't send one explicitly, but the server/host will interpret not sending one explicitly as request_id: 0 (serde does this for us).
There was a problem hiding this comment.
But what is the significance of it? Ensure that the server did not send us anything we didn't ask for?
There was a problem hiding this comment.
Yeah pretty much, and so that we don't print it.
There was a problem hiding this comment.
Uh, I'm not sure I understand what you're asking for.
The SubscriptionUpdate sent by the server contains two additional fields besides the table updates:
uint32 requestIduint64 total_host_execution_duration_micros
However, neither of them is documented nor well-specified (arguably).
So I'd argue that it is reasonable to not surface this to users unless and until stabilized.
There was a problem hiding this comment.
So I'd argue that it is reasonable to not surface this to users unless and until stabilized.
That's fair 👍
I'll update the proposal accordingly later on.
| @@ -96,11 +97,13 @@ def extract_field(cmd_output, field_name): | |||
| field, = extract_fields(cmd_output, field_name) | |||
| return field | |||
There was a problem hiding this comment.
The diff here isn't something I understood to a degree where I can approve, so let's have someone better at python review the changes here.
| match msg { | ||
| ServerMessage::SubscriptionUpdate(sub) => { | ||
| if print_initial_update { | ||
| let output = serde_json::to_string(&sub.reformat(&module_def)?)? + "\n"; |
There was a problem hiding this comment.
We should print in the same format as the spacetime sql subcommand, not as JSON.
There was a problem hiding this comment.
For the smoketests it is convenient to receive this as JSON, so perhaps we should add a --format to both sql and subscribe which accepts json, bsatn, and plain where the default is plain and formats as sql currently does.
There was a problem hiding this comment.
This seems broadly sensible. I'm not sure there's utility to the bsatn format, and I would name the current/default format table, not plain, but I think you're right that we should offer at least both of JSON and tabular output. I would also like to be careful that we use the same code for formatting in both subcommands so that we get the same format.
There was a problem hiding this comment.
Happy to skip bsatn for now until there is utility.
And I like table -- calling these --format table | json seems sensible as well.
There was a problem hiding this comment.
Note: this is slightly less trivial than expected. Because:
- the transaction updates contain deleted and inserted rows, which arguably is information we want to preserve
- the subscription update supposedly only contains inserts -- but why would we format the subscription update differently from the transaction updates?
- both of the updates refer to several tables, but it's not obvious how to display that (i.e. add some kind of "title" to a table)
What could work is to render one table per update, where the rows get two additional columns "table_name STRING" and "op ENUM('I', 'D')".
There was a problem hiding this comment.
I'll submit this as a follow-up.
Co-authored-by: Mazdak Farrokhzad <[email protected]> Signed-off-by: Kim Altintop <[email protected]>
Description of Changes
Pending proposal acceptance
Expected complexity level and risk
2