You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by dvjyothsna <gi...@git.apache.org> on 2018/03/07 02:13:10 UTC
[GitHub] drill pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
GitHub user dvjyothsna opened a pull request:
https://github.com/apache/drill/pull/1153
DRILL-6044: Fixed shutdown button in Web UI when ssl,auth are enabled
Fixed shutdown button to avoid cross domain requests since they are causing some issues when ssl and auth are enabled
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dvjyothsna/drill DRILL-6044
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1153.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 #1153
----
commit 6e215a493919b2aa6ce660e06a41ec466f8e1a78
Author: dvjyothsna <jy...@...>
Date: 2018-03-07T02:10:22Z
DRILL-6044: Fixed shutdown button in Web UI when ssl,auth are enabled
----
---
[GitHub] drill pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/1153#discussion_r173030603
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -272,6 +281,12 @@
<#if model.shouldShowAdminInfo()>
function shutdown(address,button) {
url = "http://"+address+":"+portNum+"/gracefulShutdown";
+ var ssl = $('#ssl').val();
+ url = "http://";
+ if (ssl == "ssl_enabled") {
+ url = "https://";
+ }
+ url = url+host+"/gracefulShutdown";
--- End diff --
I guess you can simplify the `shutdown` function as below.
```
function shutdown(button) {
var protocol = location.protocol;
var host = location.host;
var requestPath = "/gracefulShutdown";
var url = protocol+host+requestPath;
......
....
}
```
---
[GitHub] drill pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1153
---
[GitHub] drill pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/1153#discussion_r173371697
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -269,9 +277,12 @@
}
});
}
- <#if model.shouldShowAdminInfo() || !model.isAuthEnabled() >
- function shutdown(address,button) {
- url = "http://"+address+":"+portNum+"/gracefulShutdown";
+ <#if model.shouldShowAdminInfo() || !model.isAuthEnabled()>
+ function shutdown(button) {
+ var protocol = location.protocol;
+ var host = location.host;
+ var requestPath = "/gracefulShutdown";
+ var url = protocol+host+requestPath;
--- End diff --
How about refactoring above lines into separate function `getRequestUrl(requestPath)` and call that from both the methods (`shutdown` & `fillQueryCount`)
```
function getRequestUrl(requestPath) {
var protocol = location.protocol;
var host = location.host;
var url = protocol + host + requestPath;
return url;
}
```
---
[GitHub] drill pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/1153#discussion_r173029148
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -272,6 +281,12 @@
<#if model.shouldShowAdminInfo()>
function shutdown(address,button) {
url = "http://"+address+":"+portNum+"/gracefulShutdown";
+ var ssl = $('#ssl').val();
+ url = "http://";
+ if (ssl == "ssl_enabled") {
+ url = "https://";
--- End diff --
Why not use [location.protocol](https://www.w3schools.com/jsref/prop_loc_protocol.asp) rather than getting the info from server side ?
---
[GitHub] drill pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/1153#discussion_r173371305
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -193,7 +192,11 @@
var port = getPortNum();
var timeout;
var size = $("#size").html();
+ var host;
+ window.onload = function () {
+ host = location.host;
+ };
--- End diff --
Please remove this `function()` and `var host`.
---
[GitHub] drill issue #1153: DRILL-6044: Fixed shutdown button in Web UI when ssl,auth...
Posted by dvjyothsna <gi...@git.apache.org>.
Github user dvjyothsna commented on the issue:
https://github.com/apache/drill/pull/1153
@sohami Please review
---
[GitHub] drill pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/1153#discussion_r173031821
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -247,11 +253,14 @@
$("#row-"+i).find("#queriesCount").text("");
}
else {
- if( status_map[key] == "ONLINE") {
+ if (status_map[key] == "ONLINE") {
$("#row-"+i).find("#status").text(status_map[key]);
}
else {
- fillQueryCount(address,i);
+ var is_ssl_enabled = $('#ssl').val();
+ if (is_ssl_enabled != "ssl_enabled") {
+ fillQueryCount(address,i);
--- End diff --
`fillQueryCount` should also handle the case for Https and Http just like `shutdown`. Looks like currently with this change if SSL is enabled then we won't be able to get the queryCount of Drillbit shutting down.
Why not handle it in same way as for `shutdown` method ?
---
[GitHub] drill issue #1153: DRILL-6044: Fixed shutdown button in Web UI when ssl,auth...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/1153
+1 LGTM. Thanks for making the changes.
---
[GitHub] drill pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/1153#discussion_r173029876
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -272,6 +281,12 @@
<#if model.shouldShowAdminInfo()>
function shutdown(address,button) {
url = "http://"+address+":"+portNum+"/gracefulShutdown";
--- End diff --
please delete this line and update the `shutdown` signature since `address` is not needed anymore.
---
[GitHub] drill issue #1153: DRILL-6044: Fixed shutdown button in Web UI when ssl,auth...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/1153
+1
---