Okhttp3 initial implementation #283
Conversation
Converting tests to local server based.
Marking Radar Search as deprecated
|
This will fix #148 |
Fixes issue where runnable was running on the RateLimitExecutorDelayThread thread, allowing only one request per rate limit to go through.
Fix issue where Runnable runs on the incorrect thread
| @@ -30,21 +30,20 @@ | |||
| * text strings (e.g. "Chicago, IL" or "Darwin, NT, Australia") or as latitude/longitude | |||
| * coordinates. The Directions API can return multi-part directions using a series of waypoints. | |||
| * | |||
| * <p>See <a href="https://developers.google.com/maps/documentation/directions/intro">documentation</a>. | |||
There was a problem hiding this comment.
It's not a big deal but please note that this file has nothing to do with the change.
| ExceptionsAllowedToRetry exceptionsAllowedToRetry); | ||
|
|
||
| interface Builder { | ||
|
|
There was a problem hiding this comment.
why spacing vs not-spacing previously?
There was a problem hiding this comment.
I hit the whole code base with google-java-format to bring it into line with our code format standards. I can do that as a separate PR on the master branch and re-generate this branch to bring the noise down.
| <T, R extends ApiResponse<T>> PendingResult<T> get(ApiConfig config, Class<? extends R> clazz, | ||
| Map<String, String> params) { | ||
| Map<String, String> params) { | ||
| if (channel != null && !channel.isEmpty() && !params.containsKey("channel")) { | ||
| params.put("channel", channel); |
There was a problem hiding this comment.
Is it weird to modify the params map of the caller? In the other get() below, you just modify the actual query.
There was a problem hiding this comment.
This isn't my code, but I did sign off on bringing it into the code base. At this point I think I'll stick a TODO on it and think about how best to clean it up.
| @@ -150,7 +162,8 @@ public GeoApiContext(RequestHandler requestHandler) { | |||
| } | |||
| } | |||
|
|
|||
| // Channel can be supplied per-request or per-context. We prioritize it from the request, so if it's not provided there, provide it here | |||
| // Channel can be supplied per-request or per-context. We prioritize it from the request, | |||
| // so if it's not provided there, provide it here | |||
| if (!channelSet && channel != null && !channel.isEmpty()) { | |||
| query.append("&channel=").append(channel); | |||
There was a problem hiding this comment.
what if the query is empty?
| * | ||
| * @see java.net.URLConnection#setConnectTimeout(int) | ||
| */ | ||
|
|
There was a problem hiding this comment.
extra space for some reason?
| } | ||
|
|
||
| /** | ||
| * Sets the default write timeout for new connections. A value of 0 means no timeout. |
There was a problem hiding this comment.
I'm curious what I should be referring to with a @see here.
| /** | ||
| * Allows specific API exceptions to be retried or not retried. | ||
| */ | ||
| public Builder toggleifExceptionIsAllowedToRetry(Class<? extends ApiException> exception, |
There was a problem hiding this comment.
toggleIfException ... (fix camelcase)
There was a problem hiding this comment.
I might also call this "set". Toggle implies.. on/off automagically.
| dispatcher.setMaxRequestsPerHost(maxQps); | ||
| rateLimitExecutorService.setQueriesPerSecond(maxQps); | ||
| } | ||
| LOG.info("Request: {}", hostName + url); |
| // with requests. | ||
| LOG.warn("OkHttpRequestHandler#setQueriesPerSecond(int,int) deprecated, ignoring minimumInterval"); | ||
| this.setQueriesPerSecond(maxQps); | ||
| return new OkHttpPendingResult<T, R>(req, client, clazz, fieldNamingPolicy, errorTimeout, maxRetries, exceptionsAllowedToRetry); |
There was a problem hiding this comment.
I suspect the reformat caught this one.
|
Just that the above method had a @see but this one does not :)
…On 17 July 2017 at 16:51, Brett Morgan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/main/java/com/google/maps/GeoApiContext.java
<#283 (comment)>
:
> - */
- public GeoApiContext setProxy(Proxy proxy) {
- requestHandler.setProxy(proxy == null ? Proxy.NO_PROXY : proxy);
- return this;
+ /**
+ * Sets the default read timeout for new connections. A value of 0 means no timeout.
+ *
+ * @see java.net.URLConnection#setReadTimeout(int)
+ */
+ public Builder readTimeout(long timeout, TimeUnit unit) {
+ builder.readTimeout(timeout, unit);
+ return this;
+ }
+
+ /**
+ * Sets the default write timeout for new connections. A value of 0 means no timeout.
I'm curious what I should be referring to with a @see here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#283 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHRkK4ACEPNZZmf3CRCBNDsAgV3MSzYks5sOwRzgaJpZM4OHiXv>
.
|
|
I'm unsure of the history on these The Occam's razor explanation for the lack of |
This is a first attempt at splitting up #283
Applying `google-java-format` to the codebase.
Here's the initial sketch of converting to OkHttp3.
No rush on this review, I'm looking to land this early to mid Q3.