-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-1886] implementation z.getZeppelinJobStatus #1930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ZEPPELIN-1886] implementation z.getZeppelinJobStatus #1930
Conversation
Leemoonsoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for @cloverhearts useful new feature.
It looks great. However, while it's new feature, do you mind add a test for it?
I think you can make test similar to ZeppelinSparkClusterTest.zRunTest().
| return getJobStatus().isRunning(); | ||
| } | ||
|
|
||
| public String getStatus() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. how about change method name to name()?
Then api will be look like,
before
z. getZeppelinJobStatus().getStatus()
after
z. getZeppelinJobStatus().name()
which avoid repeating Status and more clear while it's returning name of status instead of returning Status type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leemoonsoo
:)
Thank you for your good idea.
Yes, I will.
| * @return | ||
| */ | ||
| @ZeppelinApi | ||
| public RemoteZeppelinJobStatus getZeppelinJobStatus(String noteId, String paragraphId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about remove 'Zeppelin' from method name? so using this api will looks like
before
z.getZeppelinJobStatus()
after
z.getJobStatus()
So we API becomes more consistent along with other existing APIs, they don't include 'Zeppelin' in method name, i.e.
z.run()
z.runNote()
z.show()
z.getJobStatus()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leemoonsoo Okay, I will fix it
|
Could you rebase/merge master and see if CI passes? This branch has a lot of CI build failure which is fixed by #1939 |
What is this PR for?
You can get the state of a paragraph through ZeppelinContext.
This allows you to implement code according to the paragraph condition.
related
#1799
What type of PR is it?
Feature
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1886
How should this be tested?
for example
Screenshots (if appropriate)
Questions: