[GitHub] drill pull request #1203: DRILL-6289: Cluster view should show more relevant...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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 ---