[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-23 Thread asfgit
Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/1203


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182553045
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/DrillMetrics.java ---
@@ -18,6 +18,7 @@
 package org.apache.drill.exec.metrics;
 
 import java.lang.management.ManagementFactory;
+import java.lang.management.OperatingSystemMXBean;
--- End diff --

Right. Fixed.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182553108
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -79,11 +94,15 @@
   
 
 ${drillbit.getState()}
-<#if (model.shouldShowAdminInfo() || 
!model.isAuthEnabled()) && drillbit.isCurrent() >
+Not Available
   
-   SHUTDOWN 
-  
+<#if (model.shouldShowAdminInfo() || 
!model.isAuthEnabled()) && (drillbit.isCurrent() || 
!model.isUserEncryptionEnabled()) >
--- End diff --

Fixed


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182552999
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -180,20 +180,22 @@ public void run() throws Exception {
 if (profileStoreProvider != storeProvider) {
   profileStoreProvider.start();
 }
-final DrillbitEndpoint md = engine.start();
+DrillbitEndpoint md = engine.start();
 manager.start(md, engine.getController(), 
engine.getDataConnectionCreator(), coord, storeProvider, profileStoreProvider);
-@SuppressWarnings("resource")
-final DrillbitContext drillbitContext = manager.getContext();
+DrillbitContext drillbitContext = manager.getContext();
--- End diff --

Done.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182507182
  
--- Diff: protocol/src/main/protobuf/Coordination.proto ---
@@ -18,6 +18,7 @@ message DrillbitEndpoint{
   OFFLINE = 3;
   }
   optional State state = 7;
+  optional int32 http_port = 8;
--- End diff --

Please compile protobuf for c++ client as well (Instructions are 
[here](https://github.com/apache/drill/blob/master/contrib/native/client/readme.linux)).
 It would be really helpful if you can put a note in [readme.txt of this 
package](https://github.com/apache/drill/blob/master/protocol/readme.txt) for 
future dev to compile c++ client protobufs whenever a change is made in any of 
.proto files.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182510967
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -79,11 +94,15 @@
   
 
 ${drillbit.getState()}
-<#if (model.shouldShowAdminInfo() || 
!model.isAuthEnabled()) && drillbit.isCurrent() >
+Not Available
   
-   SHUTDOWN 
-  
+<#if (model.shouldShowAdminInfo() || 
!model.isAuthEnabled()) && (drillbit.isCurrent() || 
!model.isUserEncryptionEnabled()) >
--- End diff --

Please re-visit this condition. `IsUserEncryptionEnabled()` only checks for 
encryption between JDBC/ODBC client to Drillbit path not for Https. You have to 
check it using protocol.
You can put the check something like below:
```
if(model.shouldShowAdminInfo() && (drillbit.isCurrent() || 
(!model.isAuthEnabled() && location.protocol != https))) {
showShutdownButton();
}
```


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182496734
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -180,20 +180,22 @@ public void run() throws Exception {
 if (profileStoreProvider != storeProvider) {
   profileStoreProvider.start();
 }
-final DrillbitEndpoint md = engine.start();
+DrillbitEndpoint md = engine.start();
 manager.start(md, engine.getController(), 
engine.getDataConnectionCreator(), coord, storeProvider, profileStoreProvider);
-@SuppressWarnings("resource")
-final DrillbitContext drillbitContext = manager.getContext();
+DrillbitContext drillbitContext = manager.getContext();
--- End diff --

Please revert above change.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182495572
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/DrillMetrics.java ---
@@ -18,6 +18,7 @@
 package org.apache.drill.exec.metrics;
 
 import java.lang.management.ManagementFactory;
+import java.lang.management.OperatingSystemMXBean;
--- End diff --

import not required anymore.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182502500
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -283,20 +308,114 @@
alert(errorThrown);
},
success: function(data) {
-  alert(data["response"]);
+   alert(data["response"]);

button.prop('disabled',true).css('opacity',0.5);
}
   });
   }
   }
-  
+
+  function remoteShutdown(button,host) {
--- End diff --

Done. Combined into one.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182502418
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -79,9 +91,14 @@
   
 
 ${drillbit.getState()}
+
 <#if (model.shouldShowAdminInfo() || 
!model.isAuthEnabled()) && drillbit.isCurrent() >
   
-   SHUTDOWN 
+   
SHUTDOWN 
+  
+<#elseif (model.shouldShowAdminInfo() || 
!model.isAuthEnabled()) && !drillbit.isCurrent() >
+  
--- End diff --

Done the change for checking for HTTPS. However, this commit just 
marginally fixes the UI, which (otherwise) should have a full revamp. I did 
attempt some of the details proposed in DRILL-6244, but the effort is not 
trivial.
For an unsecured setup, there is little reason to restrict the ability to 
shutdown multiple nodes, because you can always go directly and shut down those 
nodes without authentication. This simply makes it convenient to have a single 
UI window to achieve the same outcome.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182501277
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -189,11 +189,16 @@ public void run() throws Exception {
 drillbitContext.getOptionManager().init();
 javaPropertiesToSystemOptions();
 
manager.getContext().getRemoteFunctionRegistry().init(context.getConfig(), 
storeProvider, coord);
-registrationHandle = coord.register(md);
 webServer.start();
-
+//Discovering HTTP port (in case of port hunting)
+if (webServer.isRunning()) {
+  int httpPort = getWebServerPort();
+  registrationHandle = 
coord.register(md.toBuilder().setHttpPort(httpPort).build());
+} else {
+  //WebServer is not running
+  registrationHandle = coord.register(md);
+}
--- End diff --

Done.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182501230
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/DrillMetrics.java ---
@@ -60,6 +61,7 @@ private static void registerSystemMetrics() {
   REGISTRY.registerAll(new 
BufferPoolMetricSet(ManagementFactory.getPlatformMBeanServer()));
   REGISTRY.registerAll(new MemoryUsageGaugeSet());
   REGISTRY.registerAll(new ThreadStatesGaugeSet());
+  REGISTRY.registerAll(new 
CpuGaugeSet(ManagementFactory.getOperatingSystemMXBean()));
--- End diff --

Retained the GaugeSet to carry ProcessCPU and Uptime information as well. 
So, I've not replaced `registerAll`.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182500652
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -67,9 +73,15 @@
 ${i}
 ${drillbit.getAddress()}<#if 
drillbit.isCurrent()>
 Current
+<#else>
+
--- End diff --

The `shutdown()` method should be in the freemarker's IF block that checks 
for authentication. Any other method is benign and does not risk changing the 
state of the Drillbit.
Also, this method simply will pop out a new window from which that 
Drillbit's UI can be accessed. This should help if someone wishes to navigate 
to that node to, say, review logs or even submit queries with that node as the 
foreman, without having to type the address (which might have a different HTTP 
port).


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182499705
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -158,7 +158,13 @@ public void start() throws Exception {
 final int selectors = 
config.getInt(ExecConstants.HTTP_JETTY_SERVER_SELECTORS);
 final QueuedThreadPool threadPool = new QueuedThreadPool(2, 2, 6);
 embeddedJetty = new Server(threadPool);
-embeddedJetty.setHandler(createServletContextHandler(authEnabled));
+ServletContextHandler webServerContext = 
createServletContextHandler(authEnabled);
+//Allow for Other Drillbits to make REST calls
+FilterHolder filterHolder = new FilterHolder(CrossOriginFilter.class);
+filterHolder.setInitParameter("allowedOrigins", "*");
--- End diff --

Yes. CORS is basically one of the means to prevent DoS attacks. I've added 
additional filter that allows access only for `/status/metrics` path.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182499262
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -260,7 +285,7 @@
   function fillQueryCount(row_id) {
   var requestPath = "/queriesCount";
   var url = getRequestUrl(requestPath);
-  var result = $.ajax({
+   var result = $.ajax({
--- End diff --

Fixed


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-18 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r182499088
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -180,7 +180,7 @@ public void run() throws Exception {
 if (profileStoreProvider != storeProvider) {
   profileStoreProvider.start();
 }
-final DrillbitEndpoint md = engine.start();
+DrillbitEndpoint md = engine.start();
--- End diff --

Removed since the scope is anyway within this method only.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-16 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181834064
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -283,20 +308,114 @@
alert(errorThrown);
},
success: function(data) {
-  alert(data["response"]);
+   alert(data["response"]);

button.prop('disabled',true).css('opacity',0.5);
}
   });
   }
   }
-  
+
+  function remoteShutdown(button,host) {
+  var url = location.protocol + "//" + host + 
"/gracefulShutdown";
+  var result = $.ajax({
+type: 'POST',
+url: url,
+contentType : 'text/plain',
+complete: function(data) {
+alert(data.responseJSON["response"]);
+button.prop('disabled',true).css('opacity',0.5);
+}
+  });
+  }
+  
+
+  function popOutRemoteDbitUI(dbitHost, dbitPort) {
--- End diff --

We can't put this and the remaining functions within the IF block since the 
localhost operations are still valid. i.e. an authenticated client on a 
specific Drillbit can ping that Drillbit for refresh of the metrics. Only 
shutdown function needs to be managed with the IF block


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-16 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181830465
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.metrics;
+
+import java.lang.management.OperatingSystemMXBean;
+import java.util.HashMap;
+import java.util.Map;
+
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricSet;
+
+/**
+ * Creates a Cpu GaugeSet
+ */
+class CpuGaugeSet implements MetricSet {
--- End diff --

I am not sure how useful `getProcessCpuLoad` alone will be, but in addition 
with total system load it can give the idea about how Drill and other services 
on node are using the CPU.  If you plan to include another metric then in that 
case `CpuGaugeSet` class is fine. Thanks for clarification.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-13 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181510944
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.metrics;
+
+import java.lang.management.OperatingSystemMXBean;
+import java.util.HashMap;
+import java.util.Map;
+
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricSet;
+
+/**
+ * Creates a Cpu GaugeSet
+ */
+class CpuGaugeSet implements MetricSet {
--- End diff --

I thought we might want to expand the GaugeSet to carry additional metrics 
like `ProcessCpuLoad` and `ProcessCpuTime` . 
Since we can shrink the {{SHUTDOWN}} button to a symbol, we do have some 
real-estate to provide the ProcessCPULoad information as well. Would it help to 
have that? 

https://docs.oracle.com/javase/7/docs/jre/api/management/extension/com/sun/management/OperatingSystemMXBean.html#getProcessCpuLoad()



---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181254054
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -67,9 +73,15 @@
 ${i}
 ${drillbit.getAddress()}<#if 
drillbit.isCurrent()>
 Current
+<#else>
+
--- End diff --

This will only work if authentication is disabled. Shouldn't we have that 
check here to display ?


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181251700
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -79,9 +91,14 @@
   
 
 ${drillbit.getState()}
+
 <#if (model.shouldShowAdminInfo() || 
!model.isAuthEnabled()) && drillbit.isCurrent() >
   
-   SHUTDOWN 
+   
SHUTDOWN 
+  
+<#elseif (model.shouldShowAdminInfo() || 
!model.isAuthEnabled()) && !drillbit.isCurrent() >
+  
--- End diff --

the elseif check should be `(!model.isAuthEnabled && !drillbit.isCurrent() 
&& check to see if not https)`

FYI - On a bigger note I think it will be cumbersome and error prone to 
implement remote shutdown when security is enabled using WebClient side 
changes. There are few options which where discussed on how to achieve it, 
please see: [DRILL-6244](https://issues.apache.org/jira/browse/DRILL-6244). I 
would prefer if we can be consistent to do remote shutdown in same way for both 
secure and unsecure case.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181252867
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -158,7 +158,13 @@ public void start() throws Exception {
 final int selectors = 
config.getInt(ExecConstants.HTTP_JETTY_SERVER_SELECTORS);
 final QueuedThreadPool threadPool = new QueuedThreadPool(2, 2, 6);
 embeddedJetty = new Server(threadPool);
-embeddedJetty.setHandler(createServletContextHandler(authEnabled));
+ServletContextHandler webServerContext = 
createServletContextHandler(authEnabled);
+//Allow for Other Drillbits to make REST calls
+FilterHolder filterHolder = new FilterHolder(CrossOriginFilter.class);
+filterHolder.setInitParameter("allowedOrigins", "*");
--- End diff --

I am not too familiar with CORS concept but would be good to see if we can 
be more restrictive instead of just *. Also when authentication is enabled then 
with this filter, CORS request still requires authentication right ? Can you 
please confirm this?


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181161557
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/DrillMetrics.java ---
@@ -60,6 +61,7 @@ private static void registerSystemMetrics() {
   REGISTRY.registerAll(new 
BufferPoolMetricSet(ManagementFactory.getPlatformMBeanServer()));
   REGISTRY.registerAll(new MemoryUsageGaugeSet());
   REGISTRY.registerAll(new ThreadStatesGaugeSet());
+  REGISTRY.registerAll(new 
CpuGaugeSet(ManagementFactory.getOperatingSystemMXBean()));
--- End diff --

please replace `registerAll` with `register`. See comment above in 
`CpuGaugeSet`


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181252317
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -260,7 +285,7 @@
   function fillQueryCount(row_id) {
   var requestPath = "/queriesCount";
   var url = getRequestUrl(requestPath);
-  var result = $.ajax({
+   var result = $.ajax({
--- End diff --

please revert the original indentation


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181258290
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -283,20 +308,114 @@
alert(errorThrown);
},
success: function(data) {
-  alert(data["response"]);
+   alert(data["response"]);

button.prop('disabled',true).css('opacity',0.5);
}
   });
   }
   }
-  
+
+  function remoteShutdown(button,host) {
+  var url = location.protocol + "//" + host + 
"/gracefulShutdown";
+  var result = $.ajax({
+type: 'POST',
+url: url,
+contentType : 'text/plain',
+complete: function(data) {
+alert(data.responseJSON["response"]);
+button.prop('disabled',true).css('opacity',0.5);
+}
+  });
+  }
+  
+
+  function popOutRemoteDbitUI(dbitHost, dbitPort) {
--- End diff --

this and functions below should be under `<#if !model.isAuthEnabled()> 
<#/if>` block


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181179746
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -189,11 +189,16 @@ public void run() throws Exception {
 drillbitContext.getOptionManager().init();
 javaPropertiesToSystemOptions();
 
manager.getContext().getRemoteFunctionRegistry().init(context.getConfig(), 
storeProvider, coord);
-registrationHandle = coord.register(md);
 webServer.start();
-
+//Discovering HTTP port (in case of port hunting)
+if (webServer.isRunning()) {
+  int httpPort = getWebServerPort();
+  registrationHandle = 
coord.register(md.toBuilder().setHttpPort(httpPort).build());
+} else {
+  //WebServer is not running
+  registrationHandle = coord.register(md);
+}
--- End diff --

how about changing with below:
```

if (webServer.isRunning()) {
final int httpPort = getWebServerPort();
md = md.toBuilder().setHttpPort(httpPort).build();
}
registrationHandle = coord.register(md);
```


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181253785
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -283,20 +308,114 @@
alert(errorThrown);
},
success: function(data) {
-  alert(data["response"]);
+   alert(data["response"]);

button.prop('disabled',true).css('opacity',0.5);
}
   });
   }
   }
-  
+
+  function remoteShutdown(button,host) {
--- End diff --

this function is mostly same as existing `shutdown(button)`. Consider 
change that to take host parameter and use that instead.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181161322
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.metrics;
+
+import java.lang.management.OperatingSystemMXBean;
+import java.util.HashMap;
+import java.util.Map;
+
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricSet;
+
+/**
+ * Creates a Cpu GaugeSet
+ */
+class CpuGaugeSet implements MetricSet {
--- End diff --

I don't think this class is needed at all, instead just keeping the 
OperatingSystemLoad class is fine. Then in DrillMetrics we have to `register` 
that instead of calling registerAll on this class. 

Reason being we only have a single metric here not actually a set so we 
should use `register` instead of `registerAll`.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread sohami
Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181165145
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -180,7 +180,7 @@ public void run() throws Exception {
 if (profileStoreProvider != storeProvider) {
   profileStoreProvider.start();
 }
-final DrillbitEndpoint md = engine.start();
+DrillbitEndpoint md = engine.start();
--- End diff --

if you do below then you need to remove final otherwise not


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181161567
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -52,12 +52,18 @@
   
 
   #
-  Address
+  Address 
+  Heap Memory Usage 
+  Direct Memory Usage 
--- End diff --

Reduced it to
`Estimated Direct Memory ACTIVELY in use (as percent of Peak Usage)`


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181158305
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.metrics;
+
+import java.lang.management.OperatingSystemMXBean;
+import java.util.HashMap;
+import java.util.Map;
+
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricSet;
+
+/**
+ * Creates a Cpu GaugeSet
+ */
+class CpuGaugeSet implements MetricSet {
--- End diff --

It belongs to the same package where it is referenced, so i thought it 
didn't need public visibility.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181157818
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.metrics;
+
+import java.lang.management.OperatingSystemMXBean;
+import java.util.HashMap;
+import java.util.Map;
+
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricSet;
+
+/**
+ * Creates a Cpu GaugeSet
+ */
+class CpuGaugeSet implements MetricSet {
+
+  private OperatingSystemMXBean osMXBean;
+
+  CpuGaugeSet() {};
--- End diff --

It belongs to the same package where it is referenced, so i thought it 
didn't need public visibility. 


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181156759
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -79,9 +91,14 @@
   
 
 ${drillbit.getState()}
+
--- End diff --

Missed catching that without an IDEA. Thanks


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181156606
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -52,12 +52,18 @@
   
 
   #
-  Address
+  Address 
+  Heap Memory Usage 
+  Direct Memory Usage 
--- End diff --

1. The problem is that the Peak Usage can go up over time. Adding 'so far' 
indicates that this isn't a final value.
2. We could drop it, but this was meant to indicate less confusion for 
users wondering why the MaxDirectMemory value is not reflected here. It does. 
however, make it rather verbose.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181155880
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -296,12 +302,19 @@ private ConstraintSecurityHandler 
createSecurityHandler() {
   }
 
   public int getPort() {
-if (embeddedJetty == null || embeddedJetty.getConnectors().length != 
1) {
+if (!isRunning()) {
   throw new UnsupportedOperationException("Http is not enabled");
 }
 return ((ServerConnector)embeddedJetty.getConnectors()[0]).getPort();
   }
 
+  public boolean isRunning() {
--- End diff --

Agreed


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181155596
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.metrics;
+
+import java.lang.management.OperatingSystemMXBean;
+import java.util.HashMap;
+import java.util.Map;
+
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricSet;
+
+/**
+ * Creates a Cpu GaugeSet
+ */
+class CpuGaugeSet implements MetricSet {
+
+  private OperatingSystemMXBean osMXBean;
+
+  CpuGaugeSet() {};
--- End diff --

I was originally thinking of making this private, so that the instance can 
only be created with the OS Bean. I might have left it as is, thinking the 
default behaviour could be without the need to explicitly pass the bean. I'll 
make it private.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181154984
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
--- End diff --

Never noticed that the license header is actually a javadoc comment. Will 
fix this.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181033697
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -79,9 +91,14 @@
   
 
 ${drillbit.getState()}
+
--- End diff --

Please remove the comment.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181033526
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -52,12 +52,18 @@
   
 
   #
-  Address
+  Address 
+  Heap Memory Usage 
+  Direct Memory Usage 
--- End diff --

1. `Direct Memory ACTIVELY in use (as percent of Peak Usage so far)` -> 
maybe we can remove `so far`?
2. `The percentage is an estimate of the total because the Netty library 
directly negotiates with the OS for memory` -> not sure that information about 
netty is useful for the user...


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181032357
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
--- End diff --

Javadoc should be changed to comment.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181033047
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -296,12 +302,19 @@ private ConstraintSecurityHandler 
createSecurityHandler() {
   }
 
   public int getPort() {
-if (embeddedJetty == null || embeddedJetty.getConnectors().length != 
1) {
+if (!isRunning()) {
   throw new UnsupportedOperationException("Http is not enabled");
 }
 return ((ServerConnector)embeddedJetty.getConnectors()[0]).getPort();
   }
 
+  public boolean isRunning() {
--- End diff --

Method code can be simplified to one line.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181032458
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.metrics;
+
+import java.lang.management.OperatingSystemMXBean;
+import java.util.HashMap;
+import java.util.Map;
+
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricSet;
+
+/**
+ * Creates a Cpu GaugeSet
+ */
+class CpuGaugeSet implements MetricSet {
--- End diff --

Why not public...


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-12 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r181032603
  
--- Diff: 
common/src/main/java/org/apache/drill/exec/metrics/CpuGaugeSet.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.metrics;
+
+import java.lang.management.OperatingSystemMXBean;
+import java.util.HashMap;
+import java.util.Map;
+
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricSet;
+
+/**
+ * Creates a Cpu GaugeSet
+ */
+class CpuGaugeSet implements MetricSet {
+
+  private OperatingSystemMXBean osMXBean;
+
+  CpuGaugeSet() {};
--- End diff --

Why do you need default constructor?


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-10 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r180553686
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 
---
@@ -102,6 +105,7 @@ public DrillbitEndpoint start() throws 
DrillbitStartupException, UnknownHostExce
 DrillbitEndpoint partialEndpoint = DrillbitEndpoint.newBuilder()
 .setAddress(hostName)
 .setUserPort(userPort)
+.setHttpPort(httpPort)
--- End diff --

Thanks to @dvjyothsna , referred to an outdated PR #1042 for the endpoint 
registration after the web-server starts up. 


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-09 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r180296333
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 
---
@@ -102,6 +105,7 @@ public DrillbitEndpoint start() throws 
DrillbitStartupException, UnknownHostExce
 DrillbitEndpoint partialEndpoint = DrillbitEndpoint.newBuilder()
 .setAddress(hostName)
 .setUserPort(userPort)
+.setHttpPort(httpPort)
--- End diff --

That's tricky. The webserver doesn't come up until after the Endpoint is 
created and ready. One possible solution would be to perform the port hunt 
before the endpoint, and assign it to a newly instantiated webserver after the 
Endpoint is created.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-09 Thread dvjyothsna
Github user dvjyothsna commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r180268114
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 
---
@@ -102,6 +105,7 @@ public DrillbitEndpoint start() throws 
DrillbitStartupException, UnknownHostExce
 DrillbitEndpoint partialEndpoint = DrillbitEndpoint.newBuilder()
 .setAddress(hostName)
 .setUserPort(userPort)
+.setHttpPort(httpPort)
--- End diff --

Recently port-hunting capability is added to WebServer. So I think we 
should get port number from the WebServer.


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-09 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r180248133
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -289,14 +314,94 @@
   });
   }
   }
-  
+
+  function remoteShutdown(button,host) {
+  var url = location.protocol + "//" + host + 
"/gracefulShutdown";
+  var result = $.ajax({
+type: 'POST',
+url: url,
+contentType : 'text/plain',
+complete: function(data) {
+alert(data.responseJSON["response"]);
+button.prop('disabled',true).css('opacity',0.5);
+}
+  });
+  }
+  
   function getRequestUrl(requestPath) {
 var protocol = location.protocol;
 var host = location.host;
 var url = protocol + "//" + host + requestPath;
 return url;
   }
-
+
+  //Iterates through all the nodes for update
+  function reloadMetrics() {
+  for (i = 1; i <= size; i++) {
+  var address = 
$("#row-"+i).find("#address").contents().get(0).nodeValue.trim();
+  var httpPort = 
$("#row-"+i).find("#httpPort").contents().get(0).nodeValue.trim();
+  updateMetricsHtml(address, httpPort, i);
+  }
+  }
+
+  //Update memory
+  //TODO: HTTPS?
+  function updateMetricsHtml(drillbit,webport,idx) {
+var result = $.ajax({
+  type: 'GET',
--- End diff --

Makes sense. I'll set the default values to Unknown rather than zero. 


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-09 Thread dvjyothsna
Github user dvjyothsna commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r180238126
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -289,14 +314,94 @@
   });
   }
   }
-  
+
+  function remoteShutdown(button,host) {
+  var url = location.protocol + "//" + host + 
"/gracefulShutdown";
+  var result = $.ajax({
+type: 'POST',
+url: url,
+contentType : 'text/plain',
+complete: function(data) {
+alert(data.responseJSON["response"]);
+button.prop('disabled',true).css('opacity',0.5);
+}
+  });
+  }
+  
   function getRequestUrl(requestPath) {
 var protocol = location.protocol;
 var host = location.host;
 var url = protocol + "//" + host + requestPath;
 return url;
   }
-
+
+  //Iterates through all the nodes for update
+  function reloadMetrics() {
+  for (i = 1; i <= size; i++) {
+  var address = 
$("#row-"+i).find("#address").contents().get(0).nodeValue.trim();
+  var httpPort = 
$("#row-"+i).find("#httpPort").contents().get(0).nodeValue.trim();
+  updateMetricsHtml(address, httpPort, i);
+  }
+  }
+
+  //Update memory
+  //TODO: HTTPS?
+  function updateMetricsHtml(drillbit,webport,idx) {
+var result = $.ajax({
+  type: 'GET',
--- End diff --

I think its safe to not make any remote ajax requests until we figure out a 
way to perform them with BOTH ssl and auth enabled. The user might not notice 
the ajax failures and assume that we are displaying wrong info (The default 
values i.e., zero).


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-09 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r180226226
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -289,14 +314,94 @@
   });
   }
   }
-  
+
+  function remoteShutdown(button,host) {
+  var url = location.protocol + "//" + host + 
"/gracefulShutdown";
+  var result = $.ajax({
+type: 'POST',
+url: url,
+contentType : 'text/plain',
+complete: function(data) {
+alert(data.responseJSON["response"]);
+button.prop('disabled',true).css('opacity',0.5);
+}
+  });
+  }
+  
   function getRequestUrl(requestPath) {
 var protocol = location.protocol;
 var host = location.host;
 var url = protocol + "//" + host + requestPath;
 return url;
   }
-
+
+  //Iterates through all the nodes for update
+  function reloadMetrics() {
+  for (i = 1; i <= size; i++) {
+  var address = 
$("#row-"+i).find("#address").contents().get(0).nodeValue.trim();
+  var httpPort = 
$("#row-"+i).find("#httpPort").contents().get(0).nodeValue.trim();
+  updateMetricsHtml(address, httpPort, i);
+  }
+  }
+
+  //Update memory
+  //TODO: HTTPS?
+  function updateMetricsHtml(drillbit,webport,idx) {
+var result = $.ajax({
+  type: 'GET',
--- End diff --

Good catch. I believe we'll get invalid certificate errors since the 
certificate exceptions most likely are not added. 
So the question I have then is whether we should disable making **any** 
remote HTTPS calls at all, or take a chance with the hope that the invalid 
certificate +might+ have been added? 


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-09 Thread dvjyothsna
Github user dvjyothsna commented on a diff in the pull request:

https://github.com/apache/drill/pull/1203#discussion_r180196490
  
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -289,14 +314,94 @@
   });
   }
   }
-  
+
+  function remoteShutdown(button,host) {
+  var url = location.protocol + "//" + host + 
"/gracefulShutdown";
+  var result = $.ajax({
+type: 'POST',
+url: url,
+contentType : 'text/plain',
+complete: function(data) {
+alert(data.responseJSON["response"]);
+button.prop('disabled',true).css('opacity',0.5);
+}
+  });
+  }
+  
   function getRequestUrl(requestPath) {
 var protocol = location.protocol;
 var host = location.host;
 var url = protocol + "//" + host + requestPath;
 return url;
   }
-
+
+  //Iterates through all the nodes for update
+  function reloadMetrics() {
+  for (i = 1; i <= size; i++) {
+  var address = 
$("#row-"+i).find("#address").contents().get(0).nodeValue.trim();
+  var httpPort = 
$("#row-"+i).find("#httpPort").contents().get(0).nodeValue.trim();
+  updateMetricsHtml(address, httpPort, i);
+  }
+  }
+
+  //Update memory
+  //TODO: HTTPS?
+  function updateMetricsHtml(drillbit,webport,idx) {
+var result = $.ajax({
+  type: 'GET',
--- End diff --

Will the ajax call works in case of HTTPS? or will it throw invalid 
certificate error since the certificate exception is not yet added for that 
host?


---


[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...

2018-04-06 Thread kkhatua
GitHub user kkhatua opened a pull request:

https://github.com/apache/drill/pull/1203

DRILL-6289: Cluster view should show more relevant information

The commits make use of AJAX calls by the browser to all the Drillbits for 
status metrics, which are then displayed here. This works with unsecured Drill 
sessions, while secure setups will not be able to show the metrics because of 
certificate authentication.
To achieve this, the HTTP port needed to be discoverable, for which changes 
to the protobuf were done to provide the HTTP port number as well.
The Drillbit homepage has been extended to show the following:
* Heap Memory in use
* Direct Memory (actively) in use - Since we're not able to get the total 
memory held by Netty at the moment, but only what is currently allocated to 
running queries. The base memory shown is the total peak allocation by Drill 
for Direct, because the actual Direct memory held is not available. In an 
active stable system in production, this should be around 90%.
* Average (System) Load Factor reported by the OS for that node.
* Pill indicating the current Drillbit whose WebUI is being viewed 
(existing UX feature), with an option to load (in a new window) the UI from 
other Drillbits.

Information such as the User, Data and Control port numbers don't help much 
during general cluster health, so it might be worth removing this information 
if more real-estate is needed. For now, we are retaining this. 

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/kkhatua/drill DRILL-6289

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/1203.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1203


commit 94e8bbaefea7c6fe9b5729bcec3cba1dbbb3bc8f
Author: Kunal Khatua 
Date:   2018-03-26T05:03:12Z

Protobuf change to carry HTTP port info

commit 9b1513da949da870fc8d9a96472c4ca307e7d5e8
Author: Kunal Khatua 
Date:   2018-04-05T22:21:47Z

Allow CORS for access to remote Drillbit metrics

Cross-origin resource sharing (CORS) is required to ensure that the 
WebServer is able serve REST calls for status pages.

commit eb9ae7c36a26194b5a49685230e2a94282efee84
Author: Kunal Khatua 
Date:   2018-04-05T21:21:18Z

Addition of Bootstrap's Glyphicons

As part of the Bootstrap's components, this meets Apache License criteria
Remove popout.png

commit 45e4831403997661c1df578a95e3c985f64ff4f8
Author: Kunal Khatua 
Date:   2018-04-06T23:08:43Z

Materialize relevant metrics

1. Heap memory (incl usage)
2. Heap memory (incl usage)
3. Average System Load (last 1 min)
4. Option to view from other nodes (pop out)
5. Added Glyphicons

commit dd507ea95a7d0ef4904092ef4fc43ee3b44ef058
Author: Kunal Khatua 
Date:   2018-03-30T06:24:54Z

Update System Table and related tests

1. Updated System Table to show HTTP port
2. Updated unit tests




---