Skip to content
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

Merged
merged 2 commits into from
Aug 27, 2017

Conversation

codefromthecrypt
Copy link
Member

@codefromthecrypt codefromthecrypt commented Aug 24, 2017

In Zipkin v1, we had a pair of SPI interfaces: SpanConsumer
and AsyncSpanConsumer.

For example, this would define the same thing twice:

void SpanConsumer.accept(List<Span> spans);

// and

void AsyncSpanConsumer.accept(List<Span> spans, Callback<Void> spans);

in v2, it looks like this (inspired by Retrofit and OkHttp's call):

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. 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.

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Aug 24, 2017

This change still needs polish but raising for early feedback cc @anuraaga @shakuzen @llinder

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.
@codefromthecrypt
Copy link
Member Author

Did significant polishing on flight, but needs unit tests

@codefromthecrypt codefromthecrypt changed the title Adds v2 Call interface based on okhttp Adds v2 Call interface based on okhttp and retrofit Aug 25, 2017
Copy link

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat.

@codefromthecrypt codefromthecrypt merged commit ac0ba02 into master Aug 27, 2017
@codefromthecrypt codefromthecrypt deleted the call-me-back branch August 27, 2017 02:50

try {
call.enqueue(callback);
failBecauseExceptionWasNotThrown(IllegalStateException.class);
Copy link
Contributor

@anuraaga anuraaga Aug 27, 2017

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);

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Aug 27, 2017 via email

@codefromthecrypt
Copy link
Member Author

here's follow-ups needed to compete the read side of elasticsearch storage #1708

abesto pushed a commit to abesto/zipkin that referenced this pull request Sep 10, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants