Made changes to analyze execution with an external analyzer like Dr. Elephant #657
Conversation
|
@rajagopr, can you post a snapshot of the UI where this button/link appears? |
|
Attaching a snapshot for reference
|
| @@ -25,6 +25,10 @@ executor.flow.threads=30 | |||
| jetty.connector.stats=true | |||
| executor.connector.stats=true | |||
|
|
|||
| # External analyzer settings | |||
There was a problem hiding this comment.
Could you add more comment? e.g. how the %s works?
Comment out these configs. Users can uncomment and customize.
|
Could you add your recent changes to this pull request to facilitate a faster review? |
|
Ok, I have pushed my recent changes. Please review. |
| * last line, it will appear as the first button on the right. | ||
| * | ||
| * Tab focus is disabled on 'analyzerButton' by setting tabindex to -1 | ||
| * for the following reason. Whenever two buttons are displayed on the |
There was a problem hiding this comment.
This will completely disable the ability to focus on the analyzer button, right?
Would it be a better trade off to have the button show up next to the Stabs tab? This way the code reflects the same order as the order of the UI elements and tab order is more natural.
Given that I don't expect keyboard based navigation to be widely used, I am more concerned about the code readability.
There was a problem hiding this comment.
It will look like the following. Not too bad IMO.
I will go ahead with this.
|
Thanks for making the suggested changes! It looks much better now. I like the comments you added. I have a pull request pending #659 which added some unit testing facilities that can be used to test UI elements. You may consider taking advantage of that if you would like to go extra miles in testing. |
|
Please review. I have made the required changes. Pending: The UI tests. I will try to add this tomorrow. |
| case "%url": | ||
| return buildExternalAnalyzerURL(req, url, pattern); | ||
| default: | ||
| logger.debug("Pattern configured is not supported. " |
There was a problem hiding this comment.
Nice log.
Consider making it logger.error?
|
The test code looks more comprehensive and easier to understand now. I like the comments you added to the tests. Thanks! |
|
You may want to wait until I merge my pull request I mentioned earlier to take advantage of the UI test capabilities if you would like to give it a try. |
|
Sure, I will wait until the UI test framework is merged in. |
|
My pull request was merged yesterday. |
|
Thanks! I will add the test. |
rajagopr
force-pushed
the
analyze_execution
branch
from
f657af9
to
08126d2
Compare
|
Please review, I have added the UI test. |
| try { | ||
| encodedFlowExecUrl = URLEncoder.encode(flowExecutionURL, "UTF-8"); | ||
| } catch(UnsupportedEncodingException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Should this error be printed to a log?
This exception shouldn't happen though. It should have been caught by the unit test you wrote.
|
LGTM. Thanks! I have added only one small suggestion. Could you please debase and collapse the commits into one? |
The feature is turned off by default and can be enabled by configuring properties in 'azkaban.properties'. When turned on a button will appear in the flow execution details page which can query the external analyzer with the flow execution id. Please refer to 'azkaban.properties' to know more about the configuration details.
rajagopr
force-pushed
the
analyze_execution
branch
from
08126d2
to
822ae9d
Compare
|
Ok done. Please merge. Note: An unrelated test has failed on the CI build. Did not see an option to re-trigger the build without a push.
|
|
You can login using your github account to try the test again. I am not sure if you need to be a project admin to do that though. I restarted the test. |
) URLs, which are referenced by names known as a "topic" are specified in azkaban.properties For example if you want an external URL under the topic of "arbitraryName", you would put the following line into azkaban.properties: azkaban.server.external.arbitraryName.url=http://someURL.com Additionally, an external log viewer can then be specified using the parameter azkaban.server.external.logviewer.topic=arbitraryName This line would reference the URL specified earlier in this description (http://someURL.com) as an external log viewer. A similar behavior has been added for specifying external analyzers, ie. azkaban.server.external.analyzer.topic=arbitraryName Note that this commit is NOT backwards compatible and deprecates the mechanism created in (azkaban#657). That commit specifies external analyzers with the config execution.external.link.url=http://someURL.com/?%url This must now be specified as azkaban.server.external.arbitraryName.url=http://someURL.com/?${url} azkaban.server.external.analyzer.topic=arbitraryName
) URLs, which are referenced by names known as a "topic" are specified in azkaban.properties For example if you want an external URL under the topic of "arbitraryName", you would put the following line into azkaban.properties: azkaban.server.external.arbitraryName.url=http://someURL.com Additionally, an external log viewer can then be specified using the parameter azkaban.server.external.logviewer.topic=arbitraryName This line would reference the URL specified earlier in this description (http://someURL.com) as an external log viewer. A similar behavior has been added for specifying external analyzers, ie. azkaban.server.external.analyzer.topic=arbitraryName Note that this commit is NOT backwards compatible and deprecates the mechanism created in (#657). That commit specifies external analyzers with the config execution.external.link.url=http://someURL.com/?%url This must now be specified as azkaban.server.external.arbitraryName.url=http://someURL.com/?${url} azkaban.server.external.analyzer.topic=arbitraryName
When configured a button will appear in the execution details page which will query the
the external analyzer with the execution flow url.