saihemanth-cloudera commented on code in PR #6893:
URL: https://github.com/apache/hive/pull/6893#discussion_r4381122912
##########
standalone-metastore/pom.xml:
##########
@@ -464,9 +464,9 @@
<scope>runtime</scope>
</dependency>
<dependency>
- <groupId>org.apache.httpcomponents</groupId>
- <artifactId>httpcore</artifactId>
- <version>${httpcomponents.core.version}</version>
+ <groupId>org.apache.httpcomponents5</groupId>
Review Comment:
shouldn't this be `org.apache.httpcomponents.core5`? Otherwise this entry
won’t match HttpCore5 right?
##########
jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java:
##########
@@ -808,22 +812,40 @@ public boolean retryRequest(IOException exception, int
executionCount, HttpConte
return false;
}
- // requests that handles "Expect continue" handshakes. If server
received the header and is waiting for body
- // then those requests can be retried. Most basic http method methods
except DELETE are idempotent as long as they
- // are not aborted.
- protected boolean handleAsIdempotent(final HttpRequest request) {
- return !(request instanceof HttpEntityEnclosingRequest);
- }
-
// checks if the request got aborted
protected boolean requestIsAborted(final HttpRequest request) {
- HttpRequest req = request;
- if (request instanceof RequestWrapper) { // does not forward request
to original
- req = ((RequestWrapper) request).getOriginal();
+ return (request instanceof HttpUriRequest && ((HttpUriRequest)
request).isAborted());
+ }
+
+ // Retry on HTTP 401 when cookie auth is enabled: the server rejected the
+ // session cookie, so re-issue the request and let the request
interceptor
+ // add the Authorization header this time (driven by
HIVE_SERVER2_RETRY_KEY).
+ // In httpclient4 this lived on the separate
ServiceUnavailableRetryStrategy
+ // slot; in httpclient5 both I/O retry and response retry share this
single
+ // strategy, so we handle 401 here rather than as a second
setRetryStrategy(...)
+ // call (which would just overwrite this one).
+ @Override
+ public boolean retryRequest(final HttpResponse response, final int
executionCount,
+ final HttpContext context) {
+ if (cookieAuthEnabled) {
+ int statusCode = response.getCode();
+ boolean ret = statusCode == 401 && executionCount <= 1;
+ if (ret) {
+ context.setAttribute(Utils.HIVE_SERVER2_RETRY_KEY,
+ Utils.HIVE_SERVER2_CONST_TRUE);
+ }
+ return ret;
}
- return (req instanceof HttpUriRequest &&
((HttpUriRequest)req).isAborted());
+ return super.retryRequest(response, executionCount, context);
Review Comment:
I think it would be better to avoid default response retries here. Retrying
Thrift POSTs on 429/503 can execute queries or writes twice.
##########
status.txt:
##########
@@ -0,0 +1,1678 @@
+On branch thrift_upgrade_2
Review Comment:
reminder: We should remove this before merging this patch.
##########
service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java:
##########
@@ -93,7 +102,7 @@
* ThriftHttpServlet
*
*/
-public class ThriftHttpServlet extends TServlet {
+public class ThriftHttpServlet extends HttpServlet {
Review Comment:
IMO, we should preserve TServlet’s custom-header methods; changing the
superclass breaks existing callers (interoperability is breaking). what do you
think?
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/SecurityUtils.java:
##########
@@ -39,21 +39,26 @@
import org.apache.hadoop.security.token.Token;
import org.apache.hadoop.security.token.TokenIdentifier;
import org.apache.hadoop.security.token.TokenSelector;
+
+import javax.security.auth.login.AppConfigurationEntry;
+import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
+import org.apache.hc.client5.http.impl.io.BasicHttpClientConnectionManager;
+import org.apache.hc.client5.http.ssl.ClientTlsStrategyBuilder;
+import org.apache.hc.client5.http.ssl.DefaultClientTlsStrategy;
+import org.apache.hc.client5.http.ssl.DefaultHostnameVerifier;
+import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactoryBuilder;
+import org.apache.hc.client5.http.ssl.TlsSocketStrategy;
+import org.apache.hc.core5.http.config.Registry;
+import org.apache.hc.core5.http.config.RegistryBuilder;
+import org.apache.hc.core5.ssl.SSLContexts;
+import org.apache.http.conn.socket.ConnectionSocketFactory;
import org.apache.thrift.transport.THttpClient;
Review Comment:
nit: Can you remove unused imports in this file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
