Added retrofit2 module. #1419

bfg · 2017-06-07T23:31:43Z

This PR adds Retrofit2 async-http-client extras module.

Project Lombok is used to evade boilerplate.

bfg · 2017-06-08T08:06:24Z

Seems like unrelated tests are failing.

  NettyReactiveStreamsTest>ReactiveStreamsTest.testConnectionDoesNotGetClosed:95 Invalid response byte at position 12000 expected [111] but found [45]

Tests run: 503, Failures: 2, Errors: 0, Skipped: 0

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] Asynchronous Http Client Project ................... SUCCESS [  0.864 s]
[INFO] Asynchronous Http Client Netty Utils ............... SUCCESS [  2.335 s]
[INFO] Asynchronous Http Client ........................... FAILURE [03:31 min]

slandelle · 2017-06-08T08:52:03Z

This PR adds Retrofit2 async-http-client extras module.

Thanks! Being curious, why do you use AHC with Retrofit? I mean I would expect anyone going with Retrofit to go with the full Square stack and use OkHttp instead.

Project Lombok is used to evade boilerplate.

Correct me if I'm wrong, but such code generation causes stacktraces to not be aligned with source code lines, right? Also, my concern is that contributors might think this is code generation fiesta time, eg why not vavr (ex javaslang) or something else.
If possible, I'd rather go without it.

Seems like unrelated tests are failing.

Yeah, that's #1380.

slandelle · 2017-06-08T08:52:37Z

extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java

+    private void throwAlreadyExecuted() {
+        throw new IllegalStateException("This call has already been executed.");
+    }
+}


missing line feed

slandelle · 2017-06-08T08:53:05Z

.../retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactory.java

+                .request(request)
+                .build();
+    }
+}


missing line feed

slandelle · 2017-06-08T08:53:19Z

...rofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactoryTest.java

+        };
+    }
+
+}


missing line feed

slandelle · 2017-06-08T08:53:26Z

...rofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactoryTest.java

+        return e -> {
+        };
+    }
+


kill empty line

slandelle · 2017-06-08T08:53:48Z

extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java

+    private void doThrow(String message) {
+        throw new RuntimeException(message);
+    }
+}


missing line feed

slandelle · 2017-06-08T08:54:38Z

...rofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactoryTest.java

+                .build();
+
+        // then
+        Assert.assertTrue(factory.getHttpClient() == httpClient);


Use Assert static imports

Use assertEquals (everywhere it makes sense)

slandelle · 2017-06-08T08:58:27Z

...fit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpRetrofitIntegrationTest.java

+        httpClient.close();
+    }
+
+    @Test(enabled = false)


Why are all the tests in this class disabled?

They're using Github API, which is rate-limited and github may not be up all the time. Do you want me to convert this test so that it would start wiremock or similar webserver on local host and perform http calls against it?

Got it. Please have a look at the org.asynchttpclient.testserver package. Then, we might need some maven build refactoring to the test jar is visible from extra modules.

slandelle · 2017-06-08T09:02:29Z

extras/retrofit2/pom.xml

+
+    <properties>
+      <retrofit2.version>2.3.0</retrofit2.version>
+      <lombok.version>1.16.16</lombok.version>


Actually, I'd rather not use lombok in there so coding style is consistent.

I can convert all the code to normal java code, but frankly i don't think it's worth it - some other open source projects, namely spring-cloud started using lombok 2-3 years ago and it seem to work for them very well.

So, do you want me to de-lombok the code?

You can leave Lombok there. But I don't think it will ever make it into main module.

bfg · 2017-06-08T10:32:33Z

Thanks! Being curious, why do you use AHC with Retrofit? I mean I would expect anyone going
with Retrofit to go with the full Square stack and use OkHttp instead.

I find AHC most configurable http client in java world. I really like the idea of sharing single event loop group between differently configured AHC client instances, i really like it's non-blocking API and websocket support.

I don't find OKHttp as configurable and i really don't like it's async api.

But i really do like the idea of retrofit - and i would like to use proven http client engine beneath it.

Correct me if I'm wrong, but such code generation causes stacktraces to not be aligned with source
code lines, right? If so, I'm fine with using Lombock for this module, but it won't propagate to main one.

Lombok generates java code at compile time, so there is no runtime performance penalty. Stack trace line numbers may differ only with @Value annotated classes if @NonNull is being used on fields.

I use lombok daily and find it super-stable and useful. If you want me to remove lombok, that's fine with me, but i really don't think it's worth rejecting it.

bfg · 2017-06-12T11:41:04Z

Ok, @slandelle i've addressed all your code review comments and added the following functionality:

integration test suite now uses org.asynchttpclient.testserver test http server
Call and Call.Factory now allow multiple consumers to be attached for specific event types, allowing easy customization

Please review again. If you're okay with it, i will squash everything into one commit and resubmit.

slandelle · 2017-06-12T14:28:50Z

Thanks! Note that I fixed a NPE issue (see next commit).

Cheers!

bfg · 2017-06-12T14:31:37Z

Whoa! Thanks for cooperation! :-)

bfg closed this Jun 8, 2017

bfg reopened this Jun 8, 2017

Added retrofit2 module.

e28607f

slandelle requested changes Jun 8, 2017

View reviewed changes

bfg added 5 commits June 10, 2017 10:14

Added newlines at the end of files.

1e357ad

Added static method runConsumers() and cleanup

3b2cced

Introduced concept of call builder customizers, cleanup

5ac6b77

Refactored integration tests.

8744424

Added multiple consumers support for each Call event type.

2bba0b9

slandelle added the Enhancement label Jun 12, 2017

slandelle added this to the 2.1.0 milestone Jun 12, 2017

slandelle merged commit ed1aa39 into AsyncHttpClient:master Jun 12, 2017

bfg deleted the retrofit_integration branch June 12, 2017 14:31

+                      };
+                  }
+              }

                
                    No newline at end of file

Added retrofit2 module. #1419

Added retrofit2 module. #1419

Uh oh!

Conversation

bfg commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bfg commented Jun 8, 2017

Uh oh!

slandelle commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfg commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bfg commented Jun 12, 2017

Uh oh!

slandelle commented Jun 12, 2017

Uh oh!

bfg commented Jun 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bfg commented Jun 7, 2017 •

edited

Loading

slandelle commented Jun 8, 2017 •

edited

Loading

bfg commented Jun 8, 2017 •

edited

Loading