-
Notifications
You must be signed in to change notification settings - Fork 82
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
NIORequestResponseWithIDHandler #157
Conversation
111e9cf
to
c0ea9cf
Compare
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.
Looks good but can we have a few tests?
c0ea9cf
to
df01454
Compare
@glbrntt thank you, added the tests. Added all the same ones as for the regular request/response handler plus a few extra tests that test out-of-order and receiving a response that doesn't match (wrong req id). |
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.
Fab, you just need to regenerate the tests @weissi.
df01454
to
e4e596c
Compare
/// | ||
/// `NIOTaggedRequestResponseHandler` does _not_ require that the `Response`s arrive on `Channel` in the same order as | ||
/// the `Request`s were submitted. They are matched by their `requestID` property (from `NIORequestIdentifiable`). | ||
public final class NIOTaggedRequestResponseHandler<Request: NIORequestIdentifiable, |
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 don't like the naming of this channel handler because
- We use two different words (
Tagged
andIdentifiable
) for the same underlying concept - A "Tag" usually doesn't uniquely identifies something e.g. the tag system for files on apple platforms
I think we should not use Tagged but use Identifiable, Identifier or ID for all names e.g. NIOIdentifiableRequestResponseHandler
.
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.
hmm "Identifiable" sounds weird. Maybe NIORequestResponseWithIDsHandler
?
eb37dfc
to
6aba161
Compare
Motivation:
NIORequestResponseHandler
is great, for protocols where responses are in the same order as requests. Unfortunately, it doesn't work for protocols where responses may arrive out-of-order with regards to the order of the requests, such as NFS (#155).Modifications:
NIORequestResponseWithIDHandler
Result:
Ability to easily handle protocols with out-of-order responses.