[GitHub] drill pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
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...
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 pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
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...
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...
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 pull request #1153: DRILL-6044: Fixed shutdown button in Web UI when s...
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...
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 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 Date: 2018-03-07T02:10:22Z DRILL-6044: Fixed shutdown button in Web UI when ssl,auth are enabled ---
