DRILL-7582: Moved Drillbits REST API communication to the back end layer by agozhiy · Pull Request #1999 · apache/drill

agozhiy · 2020-02-26T14:24:43Z

DRILL-7582: Moved Drillbits REST API communication to the back end layer

Description

There is a list of Drillbits on the Web UI index page and some statistics, such as Heap Memory Usage, Direct Memory Usage, CPU Usage, Avg Sys Load, Uptime.
Retrieving these stats is implemented by javascript REST API calls from client side. This causes the following problems:

Requires all Drillbit hostnames to be be added to client hosts as they are used in the URLs.
Won't work with SSL/TLS enabled.
Won't work if the Drill cluster is running in isolated environment such as Kubernetes.
Won't work if Drill is running in docker and the Web port is remapped (e.g. -p 80:8047)

Now all these HTTP requests are sent by org.apache.http.impl.nio.client.CloseableHttpAsyncClient in back end.

Documentation

Statistics should be displayed in all cases.

Testing

Tested manually with SSL enabled/disabled as well as authentication.
Unit / Functional tests were passed.

vvysotskyi · 2020-02-26T18:02:29Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+   * Build an URL of a remote Drillbit endpoint.
+   *
+   * @param work {@link WorkManager} instance needed to retrieve the Drillbit address.
+   * @param request {@link HttpServletRequest} instance needed to to set the URL schema.


Please remove repeated to

vvysotskyi · 2020-02-26T18:05:43Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+        .orElse(null);
+    if (drillbit == null) {
+      throw new RuntimeException(String.format("No such drillbit: %s", hostname));
+    }


Suggested change

.orElse(null);

if (drillbit == null) {

throw new RuntimeException(String.format("No such drillbit: %s", hostname));

}

.orElseThrow(() -> new RuntimeException("No such drillbit: " + hostname));

vvysotskyi · 2020-02-26T18:07:18Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+    if (drillbit == null) {
+      throw new RuntimeException(String.format("No such drillbit: %s", hostname));
+    }
+    URL url;


No need to declare this variable, the value may be returned in the place where it is assigned.

vvysotskyi · 2020-02-26T18:08:59Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+    try {
+      url = new URL(request.getScheme(), hostname, drillbit.getHttpPort(), path);
+    } catch (MalformedURLException e) {
+      throw new RuntimeException(e);


Is there any reason for wrapping the exception into RuntimeException here?

More broadly, we have a REST call asking for a table of values. Do we want that request to fail for issues such as this (or for HTTP timeouts or other errors)? My guess is we don't. So, maybe map this kind of exception to a single form of exception which can be a) logged, and b) mapped to a value of "Unavailable" in the UI.

Actually, the malformed URL is in the programmers' control (not the user's.) So, throwing an IllegalStateException might help indicate that this is a "should never occur" kind of error.

Still need to handle network timeouts, etc.

Changed to IllegalStateException.

vvysotskyi · 2020-02-26T18:12:29Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+    }
+    URL url;
+    try {
+      url = new URL(request.getScheme(), hostname, drillbit.getHttpPort(), path);


Instead of obtaining DrillbitEndpoint and then obtaining its port, may be added additional map call into stream chain right after the filter:

.map(DrillbitEndpoint::getHttpPort)

vvysotskyi · 2020-02-26T18:39:06Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+        localHttpClient = httpClient;
+        if (httpClient == null){
+          if (drillConfig.getBoolean(ExecConstants.HTTP_ENABLE_SSL)) {
+            SSLContext sslContext = SSLContexts.custom()


Wouldn't it be better to obtain this instance using SSLConfigBuilder/SSLConfigClient and specify SslContext obtained from here instead of specifying SSLStrategy?

vvysotskyi · 2020-02-26T18:40:53Z

exec/java-exec/src/main/resources/rest/index.ftl

                        fillQueryCount(i);
                    }
-                    currentRow.find("#status").text(status_map[key]).css('font-style','').prop('title','');;
+                  currentRow.find("#status").text(status_map[key]).css('font-style', '').prop('title', '');


Something bad with indentation here.

paul-rogers · 2020-02-27T02:13:27Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+        .filter(db -> db.getAddress().equals(hostname))
+        .findAny()
+        .orElse(null);
+    if (drillbit == null) {


This is a programming invariant check. Maybe use wither assert or Preconditions.

Changed to orElseThrow.

paul-rogers · 2020-02-27T02:16:18Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+    try {
+      url = new URL(request.getScheme(), hostname, drillbit.getHttpPort(), path);
+    } catch (MalformedURLException e) {
+      throw new RuntimeException(e);


More broadly, we have a REST call asking for a table of values. Do we want that request to fail for issues such as this (or for HTTP timeouts or other errors)? My guess is we don't. So, maybe map this kind of exception to a single form of exception which can be a) logged, and b) mapped to a value of "Unavailable" in the UI.

Actually, the malformed URL is in the programmers' control (not the user's.) So, throwing an IllegalStateException might help indicate that this is a "should never occur" kind of error.

Still need to handle network timeouts, etc.

paul-rogers · 2020-02-27T02:16:53Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+  }
+
+  /**
+   * Send an HTTP request and returns response body as String.


Parallel constuctin: "Send ... and return ..." (no "s" on "return").

paul-rogers · 2020-02-27T02:17:46Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+   * @throws Exception if unable to create HTTP client or in case of HTTP timeout.
+   */
+  static String doHTTPRequest(HttpRequestBase httpRequest, DrillConfig drillConfig) throws Exception {
+    CloseableHttpAsyncClient httpClient = getHttpClient(drillConfig);


Reasonable timeout? Give Drillbit 2 seconds to respond, else display "Unavailable"?

paul-rogers · 2020-02-27T02:19:47Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUtils.java

+    if (localHttpClient == null) {
+      synchronized (WebUtils.class) {
+        localHttpClient = httpClient;
+        if (httpClient == null){


Nit: ){ --> ') {`.

More substantially, maybe separate the create-if-null hokey-pokey with actual creation. Call a method to build the client rather than doing it deeply nested here.

vvysotskyi

Thanks for making changes, +1

arina-ielchiieva requested a review from vvysotskyi February 26, 2020 14:34

vvysotskyi reviewed Feb 26, 2020

View reviewed changes

paul-rogers reviewed Feb 27, 2020

View reviewed changes

vvysotskyi approved these changes Mar 2, 2020

View reviewed changes

DRILL-7582: Moved Drillbits REST API communication to the back end layer

0f8c323

agozhiy force-pushed the DRILL-7582 branch from adcdc79 to 0f8c323 Compare March 2, 2020 12:12

asfgit closed this in 7509dc4 Mar 2, 2020

Conversation

agozhiy commented Feb 26, 2020

DRILL-7582: Moved Drillbits REST API communication to the back end layer

Description

Documentation

Testing

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!

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!

vvysotskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments