-
Notifications
You must be signed in to change notification settings - Fork 16
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
Collector API redesign #2077
Collector API redesign #2077
Conversation
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.
Nicely written!
let http_client = if let Some(http_client) = self.http_client { | ||
http_client | ||
} else { | ||
default_http_client()? | ||
}; |
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.
Q: Is having the default inserted here just so that new()
isn't fallible?
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.
It was moreso to avoid setting up and tearing down synchronization primitives, etc. for the connection pool if a separate client is supplied, and the default goes unused.
collector/src/lib.rs
Outdated
@@ -238,21 +170,30 @@ impl<P, Q: QueryType> CollectionJob<P, Q> { | |||
aggregation_parameter, | |||
} | |||
} | |||
|
|||
/// Destructure a collection job into its fields. | |||
pub fn into_fields(self) -> (CollectionJobId, Query<Q>, P) { |
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.
This method seems like it's roughly equivalent to making the three fields on the struct pub
. I get that the intent is to enable clients to work out their own serialization scheme, but I don't know if this is much better than three different getter methods. Given that this is public API, I think it might be wiser not to add this and see about adding serialization later, since that wouldn't be a breaking change.
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.
I swapped this out for normal getters
bac7250
to
19c4040
Compare
This overhauls the public API of janus_collector.
CollectorParameters
is removed, and functionally replaced with the newCollectorBuilder
.CollectionJob
andPollResult
are made public, along with methods for starting and polling a job. A new method is added to delete a job.CollectionJob
has a public constructor andinto_fields()
method, so that it can be serialized and reconstructed in stateful collectors, but I chose not to define serde implementations (orprio::codec
implementations) in this PR, since the struct has two generic parameters, mixing VDAF-level and DAP-level concepts. Implementing serialization in an opinionated way may require additional trait bounds. The constructor and access to fields at least makes it possible for applications to do their own serialization that suits their needs.Closes #1796 and closes #762.