[CALCITE-6811] Refactor deprecated httpclient API usage in Avatica

[stoty]

No description provided.

[chrisdennis]

I think I'd prefer an approach like this for adapting to the new ensureOpen(...) syntax since it avoids the need for any pass-by-instance semantics:

@Override public byte[] send(byte[] request) {
  // Create the client with the AuthSchemeRegistry and manager
  HttpPost post = new HttpPost(uri);
  try {
    HttpHost host = RoutingSupport.determineHost(post);

    while (true) {
      ByteArrayEntity entity = new ByteArrayEntity(request, ContentType.APPLICATION_OCTET_STREAM);

      post.setEntity(entity);

      try (ClassicHttpResponse response = executeOpen(host, post, context)) {
        final int statusCode = response.getCode();
        if (HttpURLConnection.HTTP_OK == statusCode
            || HttpURLConnection.HTTP_INTERNAL_ERROR == statusCode) {
          userToken = context.getUserToken();
          return EntityUtils.toByteArray(response.getEntity());
        } else if (HttpURLConnection.HTTP_UNAVAILABLE == statusCode) {
          LOG.debug("Failed to connect to server (HTTP/503), retrying");
          continue;
        }

        throw new RuntimeException(
            "Failed to execute HTTP Request, got HTTP/" + statusCode);
      } catch (NoHttpResponseException e) {
        // This can happen when sitting behind a load balancer and a backend server dies
        LOG.debug("The server failed to issue an HTTP response, retrying");
        continue;
      }
    }
  } catch (RuntimeException e) {
    throw e;
  } catch (Exception e) {
    LOG.debug("Failed to execute HTTP request", e);
    throw new RuntimeException(e);
  }
}

[stoty]

Can you elaborate @chrisdennis ?

I couldn't find ensureOpen() in either the apache HttpClient or Avatica codebase.
Google found it in some nio implementation classes, but I don't see how that's related.

[stoty]

If you mean using the execute() calls that pass a Handler, those don't return error codes, and would make implementing the retry logic unneccarily convoluted.

The response is already in a try-with-resources block, so there is no danger of leakage.

[chrisdennis]

Sorry - I've no idea why I wrote ensureOpen... you're right I mean't executeOpen(...). I'm not suggesting migrating to execute(...) calls though. I meant that if you formulate things like above then you don't need to compute the HttpHost in advance using a "dummy" HttpPost/URI, and you don't need to pass the result in via the new instance variable, you can just reorder things slightly, and keep everything contained within the call stack.

[stoty]

Thanks for the clarification.

Yes, that would work, but we're currently caching and pre-computing as much as possible.
Your version is slightly less efficient, as it needs to call RoutingSupport.determineHost for each send() call.

But we can avoid the Dummy post by postponing the host detection, I will do that.

[stoty]

I have removed the dummy post, please re-check @chrisdennis .

[stoty]

merged manually