You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Ethanlm <gi...@git.apache.org> on 2017/11/20 22:36:35 UTC
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
GitHub user Ethanlm opened a pull request:
https://github.com/apache/storm/pull/2430
[STORM-2827] fix Logviewer search returning incorrect logviewerUrl problem
https://issues.apache.org/jira/browse/STORM-2827
The logviewer search returns "http" url no matter whether logviewer is using https or not, which could lead to 404 not found error.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Ethanlm/storm STORM-2827
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2430.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 #2430
----
commit ebdd08615627c383580605d4056ca15e463c2f66
Author: Ethan Li <et...@gmail.com>
Date: 2017-11-20T22:32:05Z
[STORM-2827] fix Logviewer search returning incorrect logviewerUrl problem
----
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152412106
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
We have two `readStormConfig()`. One is https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/utils/ConfigUtils.java#L135
The other one is
https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/utils/Utils.java#L262
It seems like we are using the second one everywhere. But we are supposed to use the first one
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152365226
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
You're right. I thought Logviewer was using the same validation code as storm-client and storm-server to ensure that stormConf was valid according to the annotations in DaemonConfig (e.g. LOGVIEWER_HTTPS_PORT is configured to use validation for "@isInteger" and "@isPositiveNumber".
Do you know if there's a reason why we aren't using the usual validation (this method https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/validation/ConfigValidation.java#L710)?
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2430
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152368944
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
@Ethanlm
It can only be `null` or a non-zero positive number.
https://github.com/apache/storm/blob/61a944fd15e564482a5c55655daad9736948d961/storm-server/src/main/java/org/apache/storm/DaemonConfig.java#L353
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152371328
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
@revans2 Your code snippet is more or less what I had in mind. I think @Ethanlm has a point too though. I don't think the validation annotations are checked anywhere in the Logviewer process.
The stormConf seems to be read here https://github.com/apache/storm/blob/master/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/LogviewerServer.java#L158 but I don't see any references to ConfigValidation in storm-webapp.
I think we should consider running ConfigValidation.validateFields in the LogviewerServer main, so we can trust that the annotation constraints are enforced.
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r153232875
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
@Ethanlm I don't understand why? As far as I can tell both `@isInteger` (https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/validation/ConfigValidation.java#L160) and `@isPositiveNumber` (https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/validation/ConfigValidation.java#L450) allow nulls to pass the validation check.
The configuration we'd disallow would be setting the https port to a negative number, which seems reasonable enough to me.
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152368599
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
@srdo I think the issue is around converting the number to an Integer. If `ObjectReader.getInt` does not have a default option it will throw an exception on a null value which is the default. But I think we could make it cleaner by doing something like
```
Object httpsPort = stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT);
if (httpsPort == null) {
this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
this.scheme = "http";
} else {
this.logviewerPort = ObjectReader.getInt(httpsPort);
this.scheme = "https";
}
```
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r153233338
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
Then it should be fine. I thought `@isInteger` and `isPositiveNumber` don't allow `null` value
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152407885
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
Actually in the old clojure code we did validate it every time we loaded it.
https://github.com/apache/storm/blob/67c65e774f20d3091caed6da82d155887e966772/storm-core/src/clj/org/apache/storm/config.clj#L72-L80
Not sure what happened with the translation but it is a regression if we are not doing it any more.
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r153216647
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
It still seems not right to me. If we change `Utils.readStormConfig()` to `ConfigUtil.readStormConfig()` in `LogviewerServer`, we require the `LOGVIEWER_HTTPS_PORT` to be `@isInteger` and `@isPositiveNumber`. Then we will have to use https for logviewer in any cases and never be able to use http. See: https://github.com/apache/storm/blob/master/storm-core/src/jvm/org/apache/storm/ui/UIHelpers.java#L239
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152348839
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
It seems a little odd to me to use getInt with a default, and then check if the value is the default. Why not check if stormConf.get(...) returns null instead?
---
[GitHub] storm issue #2430: [STORM-2827] fix Logviewer search returning incorrect log...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2430
I filed a separate jira for readStormConfig() related issue because it looks like there are many places using wrong function. We need to identify them and change them.
JIRA: https://issues.apache.org/jira/browse/STORM-2832
---
[GitHub] storm issue #2430: [STORM-2827] fix Logviewer search returning incorrect log...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2430
@srdo Thanks for the review.
@revans2 @HeartSaVioR Could you please take a second look? Thanks
---
[GitHub] storm issue #2430: [STORM-2827] fix Logviewer search returning incorrect log...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2430
+1, thanks for filing the followup issue too.
---
[GitHub] storm issue #2430: [STORM-2827] fix Logviewer search returning incorrect log...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2430
The travis test passed on my repo: https://travis-ci.org/Ethanlm/storm/builds/304993981
Don't know why it failed here.
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152360979
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
It's possible that users set it to a non-positive number. We need to check them both.
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152434183
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
@Ethanlm
You're right about using which method to use: Only first one validates the fields. We should fix it, maybe better to rename `Utils.readStormConfig()` or even move the logic to ConfigUtils if we don't find any place which should use Utils.readStormConfig() instead of ConfigUtils'.
Btw, `ConfigValidation.validateFields` also validates DaemonConfig, if we have storm-server in classpath. Please refer https://issues.apache.org/jira/browse/STORM-832 and its patch https://github.com/apache/storm/pull/2065.
---
[GitHub] storm issue #2430: [STORM-2827] fix Logviewer search returning incorrect log...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2430
I think some of the tests are still flaky.
---
[GitHub] storm issue #2430: [STORM-2827] fix Logviewer search returning incorrect log...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2430
Thanks @Ethanlm, merged to master.
---
[GitHub] storm pull request #2430: [STORM-2827] fix Logviewer search returning incorr...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2430#discussion_r152392265
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java ---
@@ -97,8 +98,14 @@ public LogviewerLogSearchHandler(Map<String, Object> stormConf, String logRoot,
this.logRoot = logRoot;
this.daemonLogRoot = daemonLogRoot;
this.resourceAuthorizer = resourceAuthorizer;
-
- this.logviewerPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_PORT));
+ Integer httpsPort = ObjectReader.getInt(stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT), 0);
--- End diff --
@srdo @revans2 Thanks for the comments. It seems to me that `ConfigValidation.validateFields` is only used to validate topology configurations. It's not used to validate the confs daemons use before launching the daemons.
---