-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Adds v2 Call interface based on okhttp and retrofit #1705
Conversation
In Zipkin v1, we had a pair of interfaces, like `SpanConsumer` and `AsyncSpanConsumer`. For example, this would define the same thing twice: ```java void accept(List<Span> spans); // and void accept(List<Span> spans, Callback<Void> spans); ``` in v2, it looks like this (inspired by OkHttp's call): ```java Call<Void> accept(List<Span> spans); // then make a call like this.. call = spanConsumer.accept(spans); // to do a blocking call.. call.execute(); // to do an async call.. call.enqueue(callback); // to cancel call.cancel(); ``` The important part of this design is that it reduces the redundant interfaces. Also, libraries like retrofit prove that this design is compatible with other async abstractions like guava or rxjava. We end up making less work for users and implementors at the cost of an intermediary type: Call.
ac7111d
to
83ce92f
Compare
Did significant polishing on flight, but needs unit tests |
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.
Neat.
|
||
try { | ||
call.enqueue(callback); | ||
failBecauseExceptionWasNotThrown(IllegalStateException.class); |
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.
Since you're using assertj, just wondering any reason not to use
assertThatThrownBy(() -> call.enqueue(callback)).isInstanceOf(IllegalStateException.class);
Will play with that in next change to the file. Thx for the tip!
…On 27 Aug 2017 2:15 pm, "Anuraag Agrawal" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In zipkin/src/test/java/zipkin/internal/v2/CallTest.java
<#1705 (comment)>:
> +
+ try {
+ call.enqueue(callback);
+ failBecauseExceptionWasNotThrown(IllegalStateException.class);
+ } catch (IllegalStateException e) {
+
+ }
+ }
+
+ @test public void enqueuesOnce() throws Exception {
+ Call<Void> call = Call.create(null);
+ call.enqueue(callback);
+
+ try {
+ call.enqueue(callback);
+ failBecauseExceptionWasNotThrown(IllegalStateException.class);
Since you're using assertj, just wondering any reason not to use
assertThatThrownBy?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#1705 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD613YFk-rY-ILSdphtxFkM7Vw1omEUks5scQmYgaJpZM4PBfRH>
.
|
here's follow-ups needed to compete the read side of elasticsearch storage #1708 |
In Zipkin v1, we had a pair of interfaces, like `SpanConsumer` and `AsyncSpanConsumer`. For example, this would define the same thing twice: ```java void accept(List<Span> spans); // and void accept(List<Span> spans, Callback<Void> spans); ``` in v2, it looks like this (inspired by OkHttp's call): ```java Call<Void> accept(List<Span> spans); // then make a call like this.. call = spanConsumer.accept(spans); // to do a blocking call.. call.execute(); // to do an async call.. call.enqueue(callback); // to cancel call.cancel(); ``` The important part of this design is that it reduces the redundant interfaces. Also, libraries like retrofit prove that this design is compatible with other async abstractions like guava or rxjava. We end up making less work for users and implementors at the cost of an intermediary type: Call.
In Zipkin v1, we had a pair of SPI interfaces:
SpanConsumer
and
AsyncSpanConsumer
.For example, this would define the same thing twice:
in v2, it looks like this (inspired by Retrofit and OkHttp's call):
The important part of this design is that it reduces the redundant
interfaces. We end up making less work for users and implementors
at the cost of an intermediary type: Call.
This design is near identical to Retrofit's Call, which is itself similar to
OkHttp's call. The mild difference here is that the underlying remote
work may not always be http even if the first impl (elasticsearch)
happens to be.
Many thanks to @JakeWharton and @swankjesse for the prior art.