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/23 13:53:21 UTC
[GitHub] incubator-zeppelin pull request: [Zeppelin 689] Add AngularJS z ob...
GitHub user doanduyhai opened a pull request:
https://github.com/apache/incubator-zeppelin/pull/740
[Zeppelin 689] Add AngularJS z object and z.angularBind()
### What is this PR for?
Add client-side `z` object with method `angularBind()`
@Leemoonsoo
Compared to the original implementation, I have simplified a lot.
Now, you can only bind angular variable to one unique scope, which is the **paragraph**. I just remove the note scope and also remove the `interpreter` parameter.
Indeed, when passing a `paragraphId`, on the server-side, we can retrieve the `Paragraph` object with the `noteId` + `paragraphId` so we can now which interpreter is currently being used.
The signature of the `angularBind(varName, value, parameters)` method has also been greatly simplified:
The `parameter` is a JSON object that looks like:
```
parameters = {
paragraph: '20160126-153136_1060166247',
paragraphs: ['20160126-171914_843040190', '20160126-181556_1915782845'],
runParagraphs: false
}
```
* paragraph: paragraph id used to update the angular object repository
* paragraphs: paragraph ids used to update the angular object repository
* runParagraphs: if true, run the paragraphs listed by the properties paragraph or paragraphs. Default = true (**not implemented yet in this PR, will be implemented in another PR with the method `z.runParagraph()`**)
_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-689]**
### How should this be tested?
* 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 Angular</button>
</form>
```
* Create a second paragraph with the following code:
```scala
z.getInterpreterContext().getParagraphId()
z.angular("superhero")
````
* 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
* In the input text, put "Superman" and click on the **Bind Angular** button
* Execute the second paragraph to see that the _superhero_ variable is now set to **Superman**
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? --> **No**
* Is there breaking changes for older versions? --> **No**
* Does this needs documentation? --> **Yes**
[ZEPPELIN-635]: https://issues.apache.org/jira/browse/ZEPPELIN-635
[ZEPPELIN-689]: https://issues.apache.org/jira/browse/ZEPPELIN-689
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/doanduyhai/incubator-zeppelin ZEPPELIN-689
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/740.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 #740
----
commit b6501c77d9eb63a91e4a772acf6703603f9751a5
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-23T10:11:40Z
[ZEPPELIN-689] Add Thrift RPC method angularRegistryPush()
commit a4b5f7c3a56e96271328f05e436e6b60b3017262
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-22T16:45:34Z
[ZEPPELIN-689] Make AngularObject constructor public because of serialization issue
commit 5746cb73452f139d18df02bf310f3b622dbe20c5
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 b78d12db1b5d80bdca8d5c146c77c4f4f9ec9758
Author: DuyHai DOAN <do...@gmail.com>
Date: 2016-02-23T10:01:31Z
[ZEPPELIN-689] Implement z.angularBind() function
----
---
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-689] Add AngularJS z ob...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-197996696
@doanduyhai Thanks for this new api and improvement.
Looks good to me and merge 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-689] Add AngularJS z ob...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-195472242
@doanduyhai Do you mind add someinfo about this new api to https://github.com/apache/incubator-zeppelin/blob/master/docs/displaysystem/angular.md?
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-195781145
@Leemoonsoo
Don't worry about documentation, this will be added.
Indeed if you look at the epic **[ZEPPELIN-635]**, I split all changes into isolated tasks. I will add a task for the change to re-use paragraph Id on note import as well as another task for global documentation of those new features ( `z.angularBind()`, `z.angularUnbind()`, `z.runParagraph()`, new paragraphId display)
Today, ASF JIRA is down so I cannot create those extra JIRAs but as soon as the ASF JIRA is back online, I'll do.
I only consider the JIRA **[ZEPPELIN-635]** finished once we merge **all** sub-tasks
In the meantime, if no one has objection or other remarks, can we merge this PR so that I can progress on the other PRs ?
[ZEPPELIN-635]: https://issues.apache.org/jira/browse/ZEPPELIN-635
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-191418007
@Leemoonsoo
I had a look into the `note.json` file. Indeed we already store the paragraph id there. Is there any reason that the paragraph id change every time we import the `note.json` file back into Zeppelin ?
---
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-689] Add AngularJS z ob...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-191504454
#### two different feature in a function.
> The reason why I add the runParagraph option for z.angularBind() is that it will be used in 80% of the time. Indeed, what's the point to bind an Angular variable if you don't refresh immediately the target paragraph for quick update ? I can't see any use-case where the user bind an Angular variable and do not click on the "Run" button on the target paragraph(s) or on the whole notebook itself.
>
> If we remove the runParagraph option, to achieve the same feature, we'll need:
> ```
> <input type="text" ng-click="z.angularBind(....); z.runParagraph(...)" >
> ```
> or worst, the user should manually click on the target paragraph like in the screenshot, it's far from being user-friendly.
Idea behind AngularDisplaySystem was, give possibility to interpreter (back-end) interact with front-end. By leveraging two way binding and watcher of AngularJS, AngularDisplaySystem could able to work without introducing any Zeppelin API in front-end side.
Current api is designed like
| Actions | Front-end API | Back-end API |
| ------------- | ------------- | ------------- |
| Initiate binding | N/A | z.angularBind(var, initialValue) |
| Update value | same to ordinary angularjs scope variable | z.angularBind(var, newValue) |
| Watching value | same to ordinary angularjs scope variable | z.angularWatch(var, (oldVal, newVal) => ) |
| Destroy binding | N/A | z.angularUnbind(var) |
in my understanding, this PR trying to change it, to
| Actions | Front-end API | Back-end API |
| ------------- | ------------- | ------------- |
| Initiate binding | z.angularbind(var, initialValue) | z.angularBind(var, initialValue) |
| Update value | same to ordinary angularjs scope variable, or z.angularBind(var, newValue) | z.angularBind(var, newValue) |
| Watching value | same to ordinary angularjs scope variable | z.angularWatch(var, (oldVal, newVal) => ) |
| Destroy binding | z.angularUnbind(var) | z.angularUnbind(var) |
In this perspective, i'm not sure it's good to embed run paragraph inside of z.angularBind() function in front-end side, while corresponding api in backend side does not.
Also not every case uses angular displays system to let user click and run other paragraph.
Angular display system can be used to dynamically update front-end without re-run the paragraph like this example https://gist.github.com/granturing/a09aed4a302a7367be92.
So, to me, it's less ambiguous and more consistent to have two apis. z.angularBind() and z.runParagraph().
#### passing paragraph id.
> But this issue also applies to ZeppelinContext.runParagraph(String paragraphId) if we had chosen to use the ZeppelinContext to run paragraphs in the back-end instead of front-end so I would say this issue is not really a new issue, it has always been there.
Right, if user use just hardcoded paragraph id to call existing `ZeppelinContext.runParagraph(String paragraphId)` then the problem will be there. However, current API set provides option to write code without hardcoded paragraph id.
```
z.runParagraph(z.getInterpreterContext.getParagraphId)
```
In the similar manner,
```
<div id="myEl"></div>
<script>
var htmlElement = angular.element('#myEl')
var z = getZeppelinContext(htmlElement)
...
</script>
```
will give an option to avoid use hardcoded paragraph id.
htmlElement is html Element that created by user code in the paragraph.
That's only way i know get current paragraph id from the javascript that defined in particular paragraph.
> I think we should fix the dynamic paragraph id and persist the paragraph id at the same time as the paragraph content. As far as I can see, the paragraph id is generated using some timestamp plus a random part so there is very few chance of collision. Consequently it is possible to export a note with all the original paragraph ids and import them elsewhere
paragraph id can not be simply changed, while of some features (rest api to run paragraph, iframe link, ...) and a lot of internal code (angulardisplay system, job scheduler, etc) assumes paragraph id is immutable.
#### front-end z.angularBind() call creates/updates angularObject into all interpreters binded.
> Allow me to disagree with this idea, if there are 10 paragraphs in the note with 10 different interpreters, it will create a lot of pollution. There is no reason that we push the Angular variable to many interpreters that are not used by the target paragraph(s).
>
> Another argument against binding the variable to all interpreters used by the note is because Angular variables are persisted in note.json file so we'll create a lot of data that will be exported/re-imported and not used ....
Right, that's what i wanted to say actually :-)
#### Advantage of having API in both front-end, back-end side
> There is also a demande on the user-mailing list recently to be able to update Angular variable from the front-end:
> 
> So the demand for controlling Angular variable from the front-end is real, not just some fancy idea I have in mind.
The question from the mailing list, "pass variable defined in a angular interpreter to another paragraph which is scala interpreter", i can convert it to "how to initiate angularObject binding from the front-end code?" Because passing variable from front end to back end is already possible once they're binded.
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-190404497
Hello @Leemoonsoo
> I think it's more clear to provide z.runParagraph() than z.angularBind() has flag of runParagraph.
Having two different feature in a single function should be avoided.
The reason why I add the `runParagraph` option for `z.angularBind()` is that it will be used in 80% of the time. Indeed, what's the point to bind an Angular variable if you don't **refresh** immediately the target paragraph for quick update ? I can't see any use-case where the user bind an Angular variable and do not click on the "Run" button on the target paragraph(s) or on the whole notebook itself.
If we remove the `runParagraph` option, to achieve the same feature, we'll need:
```html
<input type="text" ng-click="z.angularBind(....); z.runParagraph(...)" >
```
or worst, the user should manually click on the target paragraph like in the screenshot, it's far from being user-friendly.
> I think it can be improved by considering two perspective. The First one is easy of use. User need to repeat paragraph id to series of z.angularBind() again and again. e.g.
I do agree with your on this point to some extent. Yes, it forces the user to do a manual copy and paste but once it's done, you don't need to update it again (we'll see this point below).
But this issue also applies to `ZeppelinContext.runParagraph(String paragraphId)` if we had chosen to use the ZeppelinContext to run paragraphs in the back-end instead of front-end so I would say this issue is not really a new issue, it has always been there.
> Second one is notebook portability, the code now depends on paragraph id and it's not portable anymore. If you export notebook and import notebook, the code will not work anymore unless user update all paragraph id in the code.
You are absolutely right about this point. Indeed it's vey annoying that exporting and importing the same note make you loose the paragraph id. For this I see 2 solutions:
1. first create the Angular `z` object from an HtmlElement as you suggest `var z = getZeppelinContext(htmlElement)`, the only problem is what is `htmlElement` ? How can we refer to a paragraph by its `htmlElement` ? I find it simpler to refer to a paragraph using its **id**
2. **I think we should fix the dynamic paragraph id and persist the paragraph id at the same time as the paragraph content**. As far as I can see, the paragraph id is generated using some timestamp plus a random part so there is very few chance of collision. Consequently it is possible to export a note with all the original paragraph ids and import them elsewhere
WDYT ?
> front-end z.angularBind() call creates/updates angularObject into all interpreters binded.
Allow me to disagree with this idea, if there are 10 paragraphs in the note with 10 different interpreters, it will create a lot of **pollution**. There is no reason that we push the Angular variable to many interpreters that are not used by the target paragraph(s).
Another argument against binding the variable to all interpreters used by the note is because Angular variables are **persisted** in `note.json` file so we'll create a lot of data that will be exported/re-imported and not used ....
> Could you explain advantage of having API in both front-end and back-end side ?
Yes, this is part of the goal **Usability Improvement** in the roadmap.
As an user, I want to be able to design a custom (with my own CSS etc ...) HTML form with my own controls (input text, drop down, calendar component, carrousel etc ...) and be able to push Angular variable to back-end.
Without this PR, to do this, I'll need to write Scala code like
```scala
import org.apache.zeppelin.display.angular.paragraphscope._
import AngularElem._
<div>
<div class="alert alert-info">This is an alert panel</div>
<form class="form-inline">
<div class="form-group">
<input type="text" class="form-control" placeholder="Input Text"></input>
</div>
<button type="submit" class="btn btn-primary">Click Me</button>
</form>
</div>.onEvent("ng-click", () => {
//my callback routine
}).display
```
If I were a web developer using Zeppelin, **I don't understand why I am forced to use Scala and the Spark interpreter to produce AngularJS and HTML source code**...
Isn't it more natural to write **plain Javascript and AngularJS code** if you want to render HTML and bind AngularJS ?
I have given many talks about Zeppelin since 6 months and everytime I build a beautiful pure HTML form using Bootstrap, people in the audience ask me why I have another paragraph with source code like this:
```scala
z.angularBind(...)
z.runParagraph(...)
```
It seems so un-natural for users to have Scala code in a separate paragraph just to make the HTML form work. If we want to extend the user-base of Zeppelin, we need to provide user natural ways of creating Angular variable and not force them to use the Spark interpreter for that.
There is also a demande on the user-mailing list recently to be able to update Angular variable from the front-end:
So the demand for controlling Angular element is real, not just some fancy idea I have in mind.
> Cons side, if user creates something that mixes both front-end side API and back-end side API at the same time, while front-end side(js in webbrowser) and back-end side (heterogeneous interpreters) runs asynchronously, i can easily imagine understanding behavior of those code will be really difficult.
Totally agree that if one user try to bind AngularJS using the back-end API with ZeppelinContext or the new Angular API you introduce and the front-end API I propose, it will make things complicated. But again, it's the user responsibility to make clean code. You can never stop people from doing stupid things right ?
To conclude, what I propose is to make another PR, before we progress on this one, **to persist the paragraph Id with the note content** so that we don't have anymore export/import problem.
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-191840961
> In this perspective, i'm not sure it's good to embed run paragraph inside of z.angularBind() function in front-end side, while corresponding api in backend side does not.
Ok, I see your point now. The table showing back-end and front-end API is quite clear. So I'll remove the `runParagraph` attribute. If user want to trigger paragraph execution they can use `z.runParagraph()`
> htmlElement is html Element that created by user code in the paragraph.
That's only way i know get current paragraph id from the javascript that defined in particular paragraph.
Your idea can work if we have a paragraph defining HTML element. But what if I want to bind Angular value to a SparkSQL paragraph or Cassandra paragraph ?
```sql
%sql
SELECT * FROM my_table WHERE id = ${id}
```
```sql
%cassandra
SELECT * FROM table WHERE key=${key}
```
There is **no** HTML element so I cannot retrieve the paragraph Id using DOM JS function... The only way is to rely on **paragraph id**, or maybe **paragraph title** but we don't have **unicity guarantee** when using title ...
> paragraph id can not be simply changed, while of some features (rest api to run paragraph, iframe link, ...) and a lot of internal code (angulardisplay system, job scheduler, etc) assumes paragraph id is immutable.
No no, I think there is a **misunderstanding** here. I don't want to change the paragraph id. What I want is that **on Import, we keep the original paragraph id** and don't generate a new one every time so that `z.angularBind('val', val, {paragraphId: 'xxxx'})` is re-usable and does not break when sharing or importing/exporting notes
To enable this behavior, **there is very few change to the code base**, see the diff **[here](https://gist.github.com/doanduyhai/350ddf29cedd4ca37f45)**
The result seems working well with import/export and clone
> Right, that's what i wanted to say actually :-)
So there is no problem, we both agree that `z.angularBind()` should **bind value to only one paragraph** right ?
> i can convert it to "how to initiate angularObject binding from the front-end code?" Because passing variable from front end to back end is already possible once they're binded.
Exactly, so `z.angularBind()` is here to fill in the gap and allow to initiate a new binding to an Angular variable from front-end.
<hr/>
So to conclude, we indeed agree on many things. I propose the following next steps:
1. Rework this PR to remove `runParagraph` so we decouple `z.angularBind()` and `z.runParagrap()`
2. Create another PR to keep the original paragraph id every time we import a note, based on the patch I showed
WDYT @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-689] Add AngularJS z ob...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-196081077
@doanduyhai Could you take a look a comment for `pushAngularObjectRegistryToRemote(client);` inside of RemoteInterpreter.init()
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-195310144
@Leemoonsoo
Is the PR now ok for merge ? I've modified it to take into account all the remarks so far
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-195792892
@Leemoonsoo
Documentation JIRA: https://issues.apache.org/jira/browse/ZEPPELIN-742
Re-use paragraphId on note import JIRA: https://issues.apache.org/jira/browse/ZEPPELIN-741
---
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-689] Add AngularJS z ob...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-192938069
> Your idea can work if we have a paragraph defining HTML element. But what if I want to bind Angular value to a SparkSQL paragraph or Cassandra paragraph ?
> ```sql
> %sql
> SELECT * FROM my_table WHERE id = ${id}
> ```
> ```sql
> %cassandra
> SELECT * FROM table WHERE key=${key}
> ```
> There is no HTML element so I cannot retrieve the paragraph Id using DOM JS function... The only way is to rely on paragraph id, or maybe paragraph title but we don't have unicity guarantee when using title
You're right in that case. Then we can just give user a tip to get paragraphId from HTML element in documentation.
> No no, I think there is a misunderstanding here. I don't want to change the paragraph id. What I want is that on Import, we keep the original paragraph id and don't generate a new one every time so that z.angularBind('val', val, {paragraphId: 'xxxx'}) is re-usable and does not break when sharing or importing/exporting notes
>
> To enable this behavior, there is very few change to the code base, see the diff here
>
> The result seems working well with import/export and clone
Understood. Sounds nice. (what happen i import the same note again?)
>>> Allow me to disagree with this idea, if there are 10 paragraphs in the note with 10 different interpreters, it will create a lot of pollution. There is no reason that we push the Angular variable to many interpreters that are not used by the target paragraph(s).
>>>
>>> Another argument against binding the variable to all interpreters used by the note is because Angular variables are persisted in note.json file so we'll create a lot of data that will be exported/re-imported and not used ....
>> Right, that's what i wanted to say actually :-)
> So there is no problem, we both agree that z.angularBind() should bind value to only one paragraph right ?
And only one interpreter.
> So to conclude, we indeed agree on many things. I propose the following next steps:
> Rework this PR to remove runParagraph so we decouple z.angularBind() and z.runParagrap()
> Create another PR to keep the original paragraph id every time we import a note, based on the patch I showed
Sounds reasonable. Thank you for all the explanation
---
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-689] Add AngularJS z ob...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-194776078
Just a quick question in case I missed something, why are we limiting the angularBind to paragraph level only?
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-194830614
@Leemoonsoo
> Understood. Sounds nice. (what happen i import the same note again?)
You can re-importe the same note many times, the note id is re-generated each time and is different for each import. But the **paragraphId** remains the same.
Remark: in the patch I mentioned above, I change the source code to re-use the paragraphId on note import BUT the jobId (the `Paragraph` class extends the `Job` class) is regenerated each time to avoid unicity issue for jobId when we re-import twice the same note ...
So in the last update of this PR, `z.angularBind('label', 'value', 'paragraphId')` only bind an Angular variable to an unique paragraph and the current interpreter used by this paragraph
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-197872773
@Leemoonsoo
Sorry, was pretty busy this week.
So regarding your remark on calling `pushAngularObjectRegistryToRemote()` only once in `RemoteInterpreter.init()`, I just add a boolean to the `InterpreterGroup` class and use it to keep track whether we push angular object registry or not for an interpreter group
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-190229721
Anyone can have a look into this PR please ? I continue rebasing it from the master to avoid conflicts when merging and I don't want to do it 10x for nothing
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-194821919
Yes @corneadoug because:
1. binding angular object at note level will require binding this value to **all** interpreters enabled in this note. This creates a lot of pollution because we save angular objects into the JSON file
2. When the user disable an interpreter for the current note, we need to unbind and remove all angular objects related to the note
3. I see few use-case where you need to bind an angular object for the whole note
---
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-689] Add AngularJS z ob...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-zeppelin/pull/740
---
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-689] Add AngularJS z ob...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-190285327
Hi @doanduyhai. Sorry for the late response. I've got some feedbacks here.
##### two different feature in a function.
I think it's more clear to provide `z.runParagraph()` than `z.angularBind()` has flag of runParagraph.
Having two different feature in a single function should be avoided.
##### passing paragraph id.
It's passing paragraph id or ids using paragraph / paragraphs field in Parameters.
I think it can be improved by considering two perspective. The First one is easy of use. User need to repeat paragraph id to series of z.angularBind() again and again. e.g.
```javascript
z.angularBind('name1', value1, { paragraph : '20160126-153136_1060166247'})
z.angularBind('name2', value2, { paragraph : '20160126-153136_1060166247'})
z.angularBind('name3', value3, { paragraph : '20160126-153136_1060166247'})
...
```
Second one is notebook portability, the code now depends on paragraph id and it's not portable anymore. If you export notebook and import notebook, the code will not work anymore unless user update all paragraph id in the code.
Considering easy of use and notebook portability, i think following style of API would help.
```javascript
// we can get paragraph id from any html element in the paragraph.
// This way, paragraph id is no longer hardcoded so notebook becomes more portable.
var z = getZeppelinContext(htmlElement)
// user can avoid passing paragraph id repeatedly.
z.angularBind('name1', value1)
z.angularBind('name2', value2)
z.angularBind('name3', value3)
```
##### front-end z.angularBind() call creates/updates angularObject into all interpreters binded.
z.angularBind() updates angularObject of all interpreters binded.
I think it should be update one interpreter who serves that particular paragraph.
##### Advantage of having API in both front-end, back-end side
It's more general question. Basically, this PR brings the API that currently only back-end side provides.
There can be Pros and Cons.
Cons side, if user creates something that mixes both front-end side API and back-end side API at the same time, while front-end side(js in webbrowser) and back-end side (heterogeneous interpreters) runs asynchronously, i can easily imagine understanding behavior of those code will be really difficult.
Not having front-end API will force user manage all angularObject in one side (back-end side), that will help simplicity of use.
Could you explain advantage of having API in both front-end and back-end side ?
---
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-689] Add AngularJS z ob...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/740#issuecomment-192927095
@Leemoonsoo what do you think about my last proposal ?
---
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.
---