various unrelated small fixes and tweaks #1752
Conversation
…gation Use some concurrency by default, and increase the heartbeat interval. Log when we're waiting for DNS propagation.
If there are many historical logs in the stream, you can get a lot of output when reading from the beginning of time and the operator is usually only interested in what's happening right now.
We were not properly tearning down these instances, which lead to lingering open RocksDB instances and open file descriptors to removed files, preventing disk from being reclaimed.
Simplify Router by starting a Channel immediately, and instead emphasize RPC timeouts rather than connection timeouts. There are a variety of conditions where connection timeouts don't really work, such as when a server has bound its port but is not actively serving it. Currently this causes tonic to block indefinitely. Also, Channel embeds a reconnection behavior if the transport is broken, so it's already the case that Channels are connecting in the background and we should expect to see corresponding delays. So, have a single behavior for Channels: they start immediately but may have connection delay in the background. - Back out connection-readiness route selection in favor of random balancing. - Distinguish local vs non-local Channels: in the future, we should use this to implement selective compression over non-local Channels. Also, upon an error a Read stream must clear its ReadRequest Header. The rationale is that the current Header could point to a Gazette broker which has permanently stopped, and connections will time out repeatedly. Clearing the Header causes route discovery to restart using the base service address.
jgraettinger
added
the
change:unplanned
Lead with an identity-transform example SELECT JSON($flow_document) Continue to emit an example selecting individual fields, but semantically disable it and also remove root document projections, because these are never desired.
| .send(logs::Line { | ||
| token: state.logs_token, | ||
| stream: "controller".to_string(), | ||
| line: format!("Waiting {DNS_TTL:?} for DNS propagation before continuing."), |
There was a problem hiding this comment.
nit: might be nice to have at least some greppable difference between these two logs messages, just to make it easier in the future to figure out which codepath a particular log message came from. The enum variant descriptions for Status::AwaitDNS1 and Status::AwaitDNS2 seem like good candidates imo
crates/gazette/src/lib.rs
Outdated
| // Note this connect_timeout accounts only for TCP connection time and | ||
| // does not apply to time required for TLS or HTTP/2 transport start, | ||
| // which can block indefinitely if the server is bound but not listening. | ||
| // Callers MUST implement per-RPC timeouts if that's important. |
There was a problem hiding this comment.
What's the right way to implement per-RPC timeouts for server-streaming RPCs, where waiting for long periods of time with no messages is a perfectly legitimate usage pattern? That is, what signal do you time out on?
There was a problem hiding this comment.
Pushed a small update here, which also relates to an issue we saw with a reactor connection to flow-connector-init which was ESTABLISHED even though the network namespace had been completely torn down.
Now the advice would be you don't need to worry about it, because HTTP/2 keep-alive verifies the server is up and actively serving the HTTP/2 transport. If you choose to apply a timeout on individual stream receives, you can still do so for each stream receive future.
HTTP/2 keep-alive sends a PING frame every interval, and fails the connection of the peer doesn't respond in time. This verifies the end-to-end health of the HTTP/2 transport and catches issues like servers which have bound sockets but aren't actively listening. Also using HTTP/2 keep-alive when connecting to local containers. We've observed that `podman` can fail in ways that leave the reactor believing it has an established connection to flow-connector-init, even though the container has failed and the network namespace has been torn down.
Description:
This is a stack of small, unrelated minor fixes and improvements. Please see individual commits.
Workflow steps:
(How does one use this feature, and how has it changed)
Documentation links affected:
(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)
Notes for reviewers:
(anything that might help someone review this PR)
This change is