-
Notifications
You must be signed in to change notification settings - Fork 184
RUST-2203 Allow client metadata to be appended post-construction #1471
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
base: main
Are you sure you want to change the base?
Conversation
pub(crate) async_event_listener: Option<TestEventSender>, | ||
|
||
/// Callback to receive hello commands. | ||
pub(crate) hello_cb: Option<EventHandler<crate::cmap::Command>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prose tests required direct visibility of hello commands, so new test hook. I used EventHandler
for this because that's easier than typing Arc<Box<dyn Fn(crate::cmap::Command)>>
all over the place and it already implements things like Debug
.
|
||
impl From<&ClientOptions> for EstablisherOptions { | ||
fn from(opts: &ClientOptions) -> Self { | ||
impl From<&TopologySpec> for EstablisherOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Establisher
/ Handshaker
now require a value outside of ClientOptions
to construct, I introduced TopologySpec
to capture that (and anything similar that happens to be needed in the future) rather than thread through another function parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were just construction tests of metadata values and IMO didn't have enough utility to be worth updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just FYI, I was confused about the ambiguity that comes with omitting unspecified fields, but looks like there's an open DRIVERS ticket to address this blocked on changes from analytics: https://jira.mongodb.org/browse/DRIVERS-3251
src/cmap/establish/handshake.rs
Outdated
} | ||
|
||
impl ClientMetadata { | ||
pub(crate) fn append(&mut self, driver_info: DriverInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec has the following requirement:
All strings provided as part of the driver info MUST NOT contain the delimiter used for metadata concatention. Drivers MUST throw an error if any of these strings contains that character.
So we should be checking for "|" in these strings - I think we're also missing that for metadata included in ClientOptions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, added.
Co-authored-by: Isabel Atkinson <[email protected]>
RUST-2203 / RUST-2272
This required moving metadata from something that each worker has its own copy of to a central per-
Client
(actually per-Topology
) shared value.I initially implemented this as an update that gets fanned out to the workers using the existing infrastructure that's there for shutdown and the like; that turned out to require a centralized copy anyway so I dropped the fan-out. In the process, though, I did some refactoring of the related methods so that worker commands require fewer intermediate pass-through wrapper methods and division of logic among the components is clearer.