You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by rekhajoshm <gi...@git.apache.org> on 2015/01/19 06:07:08 UTC
[GitHub] storm pull request: STORM-469
GitHub user rekhajoshm opened a pull request:
https://github.com/apache/storm/pull/385
STORM-469
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rekhajoshm/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/385.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 #385
----
commit 2931487f0cbcb67992e67deba40bb5fe68261fd6
Author: Joshi <rj...@mtvl122dc9fdc.local>
Date: 2015-01-19T05:07:07Z
STORM-469
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-469
Posted by rekhajoshm <gi...@git.apache.org>.
Github user rekhajoshm closed the pull request at:
https://github.com/apache/storm/pull/385
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-469
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/385#issuecomment-70666765
@rekhajoshm Ok. Open up a different pull request for each of those JIRAs. Once the PR approved by the committers we will merge pr directly into master branch. combining two JIRAs into one PR is not recommended.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-469
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/385#issuecomment-70523175
-1. I don't think the change is necessary. Storm ships with a logviewer daemon. If users want more details on some exception in ui, they have the ability to do so by ensuring that they have the logviewer daemons started which should allow them to navigate and view the entire log file through ui.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-469
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/385#issuecomment-70574408
@rekhajoshm Thanks for the patch. Few observations here
1) you are changing interfaces to pass a codeDir param. I can't find it where codeDir used to fixed this issue. Whenever you are changing apis like this you should introduce a new method definition not change the existing ones as these are storm-core apis. Please also explain whats the necessity of codeDir.
2) you added dependency-reduced-pom.xml and any reason to remove org.clojars.runa from pom.xml. Also maven-eclipse.xml shouldn't be part of this commit.
I am -1 on the current approach, changing apis to solve this is not a good idea. This pretty much breaks current topologies.
I suggest you to look into ui/core.clj , one approach is to have a search functionality in logviewer and provide a link next to error message with param to logviewer as the errormessage. When user clicks on that it goes to logviewer and highlight the error in the log file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-469
Posted by rekhajoshm <gi...@git.apache.org>.
Github user rekhajoshm commented on the pull request:
https://github.com/apache/storm/pull/385#issuecomment-70571273
jmlogan: you are right about the extra files.few more changes.will update commit.thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-469
Posted by rekhajoshm <gi...@git.apache.org>.
Github user rekhajoshm commented on the pull request:
https://github.com/apache/storm/pull/385#issuecomment-72723038
created separate pulls.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-469
Posted by rekhajoshm <gi...@git.apache.org>.
Github user rekhajoshm commented on the pull request:
https://github.com/apache/storm/pull/385#issuecomment-70607612
Thanks Harshch for having a look.Somehow you seem to have reviewed both storm-469 and storm-528 commits together?
1. commit 2931487 is for storm-469, tested and works on my storm cluster and gives me link to truncated text the log link, which works.
2. commit ef19e71 is for storm-528 and as informed to jmlogan, i will update the files in a while.Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-469
Posted by rekhajoshm <gi...@git.apache.org>.
Github user rekhajoshm commented on the pull request:
https://github.com/apache/storm/pull/385#issuecomment-70570930
Parth-Brahmbhatt: As you like it, Parth :-)
IMO, if a truncated errormsg is shown , a link on it to the logs is a good ui design., and more user friendly.
Agree with Jason Kania on it.It might be even cooler to show last few bytes of log link, or complete log link…as in hadoop jobtracker design.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---