-
Notifications
You must be signed in to change notification settings - Fork 154
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
Pull Consumer builder #541
Conversation
self | ||
} | ||
|
||
pub fn expires(mut self, expires: Duration) -> Self { |
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.
We know there is a bug with expires
in the server, but I still think we should expose that methods, as it's not client err. Also, not making it public forces us to remove (ignored) timeout test.
consumer: &'a Consumer<Config>, | ||
} | ||
|
||
impl<'a> StreamBuilder<'a> { |
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.
Alternatively, since we store the configuration already in Stream.
We can consume it and have these methods act as if they're combinators?
(Devil's advocate, not sure if I prefer this or that yet)
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.
Was thinking about that, especially when started writing tests and using it, but I think it's better to not consumer.
Otherwise Stream builder would act differently than Batch, and we ... might want to allow calling stream
again after error happening? Maybe, maybe no, but if we consume, its a no for sure.
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.
Yeah I'm unsure too, lets start with this and iterate over the next couple of releases.
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.
lgtm
90addeb
to
d47f2ee
Compare
No description provided.