You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by doanduyhai <gi...@git.apache.org> on 2016/02/24 16:22:06 UTC
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-697] Replace dynamic fo...
GitHub user doanduyhai opened a pull request:
https://github.com/apache/incubator-zeppelin/pull/745
[ZEPPELIN-697] Replace dynamic form with angular object from registry if exists
### What is this PR for?
Replace dynamic form with angular object from registry if exists
I updated the `Paragraph.jobRun()` method to look for existing variable from the Angular Object Registry first before displaying the dynamic form.
We look for Angular object having same name:
* first at paragraph scope (note id + paragraph id)
* then at note scope (note id only)
I did not look at **global** scope because @Leemoonsoo was mentioning somewhere that we may likely remove the global scope some day
_This is a sub-task of epic **[ZEPPELIN-635]**_
### What type of PR is it?
[Improvement]
### Todos
* [ ] - Code Review
* [ ] - Simple Test
### Is there a relevant Jira issue?
**[ZEPPELIN-697]**
### How should this be tested?
* `git fetch origin pull/745/head:AngularObjectReplaceDynamicFormVar`
* `git checkout AngularObjectReplaceDynamicFormVar`
* `mvn clean package -DskipTests`
* `bin/zeppelin-daemon.sh restart`
* Create a new note
* In the first paragraph, put the following code
```html
%angular
<form class="form-inline">
<div class="form-group">
<label for="superheroId">Super Hero: </label>
<input type="text" class="form-control" id="superheroId" placeholder="Superhero name ..." ng-model="superhero"></input>
</div>
<button type="submit" class="btn btn-primary" ng-click="z.angularBind('superhero', superhero, {paragraph: 'PUT_HERE_PARAGRAPH_ID'})"> Bind</button>
<button type="submit" class="btn btn-primary" ng-click="z.angularUnbind('superhero', {paragraph: 'PUT_HERE_PARAGRAPH_ID'})"> Unbind</button>
</form>
```
* Create a second paragraph with the following code:
```scala
z.getInterpreterContext().getParagraphId()
```
* Execute the second paragraph to retrieve its paragraph id
* In the first paragraph, replace the text PUT_HERE_PARAGRAPH_ID by the correct paragraph id
* Update the second paragraph content to
```
%md
#### Super Hero of the day: **${superhero}**
```
* Now put **SpiderMan** in the input text of the first paragraph and click alternatively on **Bind** and **Unbind**
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? --> **No**
* Is there breaking changes for older versions? --> **Yes**
* Does this needs documentation? --> **Yes**
[ZEPPELIN-635]: https://issues.apache.org/jira/browse/ZEPPELIN-635
[ZEPPELIN-697]: https://issues.apache.org/jira/browse/ZEPPELIN-697
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/doanduyhai/incubator-zeppelin ZEPPELIN-697
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/745.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 #745
----
commit f221238b6b0bdff889bc996742ac10348eb4a081
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-22T16:45:34Z
[ZEPPELIN-689] Make AngularObject constructor public because of serialization issue
commit bfe1a22aeae9178d4a3bf5e8d53fda525ac5f6cd
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-22T22:28:11Z
[ZEPPELIN-689] ZeppelinContext angular() method should look for variable using the paragraph scope then note scope
commit c11b86c6815f510d71490edad311aab0484d2ac5
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-23T10:11:40Z
[ZEPPELIN-689] Add Thrift RPC method angularRegistryPush()
commit d81d06a85b14c4704dc5c676301ac8137eabaa0e
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-23T10:01:31Z
[ZEPPELIN-689] Implement z.angularBind() function
commit 1592c2966b4c4678f26f09bb794516e1ac8a2475
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-23T14:20:46Z
[ZEPPELIN-693] Add AngularJS z.angularUnbind()
commit eeaf137ab5ec08be4d047f34e5787879ac1e649f
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-23T15:07:36Z
[ZEPPELIN-695] Add AngularJS z.runParagraph()
commit 95d08c90411dcae3c57ba0bc0a42455b115a4b7d
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-23T22:33:18Z
[ZEPPELIN-696] Add notification system for AngularJS z functions
commit c1cd3d0418290045201230506278b7aee22ba4dc
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-24T15:12:11Z
[ZEPPELIN-697] Replace dynamic form value with Angular variable if exists
----
---
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] incubator-zeppelin pull request: [ZEPPELIN-697] Replace dynamic fo...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/745#issuecomment-206705438
The implementation itself looks good to me.
While this PR links 'dynamic form' and 'angular display system', I'd like to discuss little bit about the direction.
Zeppelin has 'dynamic form' and 'angular display system' for frontend - backend interaction.
And 'resource pool' for interpreter-interpreter interaction. I can summarize characteristics of these three, like
| | Dynamic Form | Angular Display System | ResourcePool |
| ------------- | ------------- | ------------- | ------------- |
| Propagate value to the font-end | yes | yes | no |
| Persisted in note.json | yes | yes | no |
| Trigger actions on change | yes | yes | no |
| Store arbitrary java object | no | yes | yes |
| Note / Paragraph scope | no | yes | yes |
| Access value between interpreters | no | no | yes |
'Dynamic Form' and 'Angular Display System' are both does frontend-backend interaction.
'Angular Display System' and 'Resource pool' are both object store.
While they're sharing similar characteristics, do you see any possibilities of consolidate these 3 into 1?
If it is, how this PR can be aligned?
---
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] incubator-zeppelin pull request: [ZEPPELIN-697] Replace dynamic fo...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/745#issuecomment-208084967
Merge into master if there're no more discussions.
---
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] incubator-zeppelin pull request: [ZEPPELIN-697] Replace dynamic fo...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/745#issuecomment-205170371
ping @Leemoonsoo for code review
---
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] incubator-zeppelin pull request: [ZEPPELIN-697] Replace dynamic fo...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/745#issuecomment-208034624
@Leemoonsoo Merge if ok ? Next 2 PR will concentrate on keeping same paragraph Id when re-importing node from JSON and add comprehensive documentation for the new AngularJS `z` functions
---
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] incubator-zeppelin pull request: [ZEPPELIN-697] Replace dynamic fo...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/745#issuecomment-207614870
I agree, consolidate Angular Object / Resource Pool into Resource Pool really make sense. I think watcher also can be handled.
Also make sense Dynamic form doesn't really need to be consolidated.
I think the way how this links PR Resource Pool and Dynamic Form make sense and aligned with the future plan.
Thanks for the patch and sharing the idea. LGTM
---
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] incubator-zeppelin pull request: [ZEPPELIN-697] Replace dynamic fo...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/745#issuecomment-206262470
ping @Leemoonsoo
---
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] incubator-zeppelin pull request: [ZEPPELIN-697] Replace dynamic fo...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/745#issuecomment-207546826
Hello @Leemoonsoo
Thank for raising question about the 3 types of variables.
My though is that Angular Variables should be merged/replaced by ResourcePool. Indeed the Angular Object scope is not very intuitive. It is first scoped by **interpreter group** then by note/paragraph.
Removing Angular Object to use Resource Pool is a good idea but there is still one pending issue: angular value watcher. As you know people can assign a watcher function in the back-end on Angular value. If we want to replace Angular objects by Resource Pool, we need to provide similar feature, unless you think we can completely drop this watcher feature ...
Another point to take into account is the **migration path**. For all people that have saved their notes as JSON with saved Angular objects, we need to handle the migration to Resource pool when the re-import those notes.
About the Dynamic Form system I don't think we should merge it with Angular Object/Resource Pool. Dynamic Form values are just an alias to automatically generate a form input. And the fact that its value change triggers the execution of the current paragraph can be implemented using `z.runParagraph()` Angular function that is now available.
All in all I think we should ultimately have enhanced Resource Pool and Dynamic Form only.
But those changes are very big and deserve another JIRA epic. For now, we can just implement this overriding system for this PR and then when we refactor the Angular JS to Resource Pool, we will merge the behavior together
WDYT ?
---
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] incubator-zeppelin pull request: [ZEPPELIN-697] Replace dynamic fo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-zeppelin/pull/745
---
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.
---