Skip to content

Conversation

@HeartSaVioR
Copy link
Contributor

What is this PR for?

To add 'progress' to 'get notebook job' REST API.
All paragraphs which status is 'running' will include 'progress' to their job status.

What type of PR is it?

Improvement

Todos

Is there a relevant Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-528

How should this be tested?

  1. Run a notebook or paragraph which takes some times. Paragraph which could show progress (%) would be more appreciated. Confirm that its status is 'RUNNING'.
  2. Call http://<zeppelin host>:<zeppelin port>/api/notebook/job/<notebook id> to see if it has 'progress' field in JSON.

Screenshots (if appropriate)

notebook-2b68v95jm
notebook-job-api-result-2b68v95jm

Questions:

  • Does the licenses files need update? (No)
  • Is there breaking changes for older versions? (No)
  • Does this needs documentation? (Yes or no. Output of the JSON format could be updated, but it could be treated as not mandatory.)

* It would be hard to trap RUNNING state, so I can't add UT
@HeartSaVioR
Copy link
Contributor Author

Two questions here,

  • Can I trap RUNNING status of the paragraph in UT? I don't have an idea to make it, so I can't add UT for now.
  • Would it be better to modify doc's output of the JSON format to include 'progress' field?

@Leemoonsoo
Copy link
Member

Thanks for the good improvement.
I think it's better add an example of the JSON output that includes 'progress' field.
Could you give me a hint that what 'UT' is?

@HeartSaVioR
Copy link
Contributor Author

UT = Unit test :)
I'll address the doc and let you know when complete.

@HeartSaVioR
Copy link
Contributor Author

@Leemoonsoo Addressed your comment.

@Leemoonsoo
Copy link
Member

Thanks! LGTM

If you would like to add UT in ZeppelinRestApiTest, you can add a such paragraph %sh sleep 1 instead of https://github.com/apache/incubator-zeppelin/blob/master/zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java#L401 and state transition READY->PENDING->RUNNING->FINISHED will stay in RUNNING state for a second. Does it make sense ?

@HeartSaVioR
Copy link
Contributor Author

@Leemoonsoo
Thanks for sharing the idea! It works like a charm!
But new unit test requires ZEPPELIN-527 to be merged since API returns HTTP STATUS 500 before applying ZEPPELIN-527.

There's a bit conflict between ZEPPELIN-527 and ZEPPELIN-528 so I'll wait ZEPPELIN-527 to be merged, and upmerge & add commit regarding UT to ZEPPELIN-528.

Could you please take a look at ZEPPELIN-527 first? Thanks!

Conflicts:
	zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java
@HeartSaVioR
Copy link
Contributor Author

@Leemoonsoo Thanks for merging ZEPPELIN-527! Upmerged and add UT.

@Leemoonsoo
Copy link
Member

Great. Looks good to me.
Merge if there're no more discussions

@asfgit asfgit closed this in f78d309 Dec 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants