You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by astroshim <gi...@git.apache.org> on 2015/11/30 11:40:35 UTC
[GitHub] incubator-zeppelin pull request: synchronization saving notebook.
GitHub user astroshim opened a pull request:
https://github.com/apache/incubator-zeppelin/pull/489
synchronization saving notebook.
The issue related to https://issues.apache.org/jira/browse/ZEPPELIN-473.
Saving notebook operation has called save method in the VFSNotebookRepo class in the end, It can be synchronized.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/astroshim/incubator-zeppelin ZEPPELIN-473
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/489.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 #489
----
commit 8c0fac8d78f79c9d13b282a3f1146530f62c0348
Author: astroshim <hs...@nflabs.com>
Date: 2015-11-30T10:33:35Z
synchronization saving notebook.
----
---
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: synchronization saving notebook.
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-161515630
thanks for the fix and tests! 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: synchronization saving notebook.
Posted by astroshim <gi...@git.apache.org>.
Github user astroshim commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160884961
@bzz Thanks for replying.
The code is the VFSNotebookRepoTest.java but i didn't push yet.
I'm wondering is that can't show the unsynchronized version test because save() method is already synchronized.
So i'm going to push the synchronized version testcase.
What do you think?
---
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: synchronization saving notebook.
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160907588
I think that is perfectly fine - we only need to test that the new code passes the test (for synchronized version). It's assumed that old code breaks the same test (which it does indeed, as you showed).
Looks great to me, :+1: for adding a tests!
---
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: synchronization saving notebook.
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160621970
Great stuff, just recently noticed that with @Leemoonsoo
Do you think just save is enough? There is also a delete method.
Also, how do you think, would it be hard to implement a test for this change?
---
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: synchronization saving notebook.
Posted by astroshim <gi...@git.apache.org>.
Github user astroshim commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-161515202
@bzz Okay. I will.
Thank you very much for your help~:)
---
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: synchronization saving notebook.
Posted by astroshim <gi...@git.apache.org>.
Github user astroshim commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160642541
@bzz
Thanks for comment.
I thought delete mothod can be needed the synchronization but because of delete method just occur exception when it is failed so save is enough. What do you think?
And it would be hard to implement test for this. Do you have any idea for testing?
---
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: synchronization saving notebook.
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160867681
@astroshim this looks awesome, I think its a great idea of the test!
If you push this code to a new `VFSNotebookRepoTest.java`, I can help converting this to the test-case i.e by:
- create a new notebook, as in [NotebookRepoSyncTest.java#L147](https://github.com/apache/incubator-zeppelin/blob/master/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java#L147)
- then run the same code as you have, but may be breaking after N milliseconds or just after M iterations (enough to break unsynchronized version)
What do you think?
---
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: synchronization saving notebook.
Posted by astroshim <gi...@git.apache.org>.
Github user astroshim commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160859692
I implemented some code for testing.
As you can see, In the while() if saving note without synchronization, note.json is crashed. (disappeared note.json file.)
But synchronized save() is fine.
---
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: synchronization saving notebook.
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-161504553
Looks good to me, great job @astroshim !
May be removing useless TODO is very minor nit ;)
BTW to trigger a CI built - you can just close and re-open this PR.
---
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: synchronization saving notebook.
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160910823
CI fails with
```
[INFO] Zeppelin: web Application .......................... SUCCESS [ 48.791 s]
[INFO] Zeppelin: Server ................................... FAILURE [04:08 min]
[INFO] Zeppelin: Packaging distribution ................... SKIPPED
....
Running org.apache.zeppelin.ZeppelinIT
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 73.106 sec <<< FAILURE! - in org.apache.zeppelin.ZeppelinIT.testAngularDisplay(org.apache.zeppelin.ZeppelinIT) Time elapsed: 73.005 sec <<< ERROR!
org.openqa.selenium.TimeoutException: Timed out after 60 seconds waiting for org.apache.zeppelin.ZeppelinIT$2@464f0728
Driver info: org.openqa.selenium.firefox.FirefoxDriver
Capabilities [{platform=LINUX, acceptSslCerts=true, javascriptEnabled=true, cssSelectorsEnabled=true, databaseEnabled=true, browserName=firefox, handlesAlerts=true, nativeEvents=false, webStorageEnabled=true, rotatable=false, locationContextEnabled=true, applicationCacheEnabled=true, takesScreenshot=true, version=31.0}]
at sun.reflect.GeneratedConstructorAccessor12.newInstance(Unknown Source)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:526)
at org.openqa.selenium.remote.ErrorHandler.createThrowable(ErrorHandler.java:204)
at org.openqa.selenium.remote.ErrorHandler.throwIfResponseFailed(ErrorHandler.java:156)
at org.openqa.selenium.remote.RemoteWebDriver.execute(RemoteWebDriver.java:599)
at org.openqa.selenium.remote.RemoteWebDriver.findElement(RemoteWebDriver.java:352)
at org.openqa.selenium.remote.RemoteWebDriver.findElementByXPath(RemoteWebDriver.java:449)
at org.openqa.selenium.By$ByXPath.findElement(By.java:357)
at org.openqa.selenium.remote.RemoteWebDriver.findElement(RemoteWebDriver.java:344)
at org.apache.zeppelin.ZeppelinIT$2.apply(ZeppelinIT.java:143)
at org.apache.zeppelin.ZeppelinIT$2.apply(ZeppelinIT.java:141)
at org.openqa.selenium.support.ui.FluentWait.until(FluentWait.java:208)
at org.apache.zeppelin.ZeppelinIT.waitForParagraph(ZeppelinIT.java:141)
at org.apache.zeppelin.ZeppelinIT.testAngularDisplay(ZeppelinIT.java:189)
Caused by: org.openqa.selenium.NoSuchElementException: Unable to locate element: {"method":"xpath","selector":"//div[@ng-controller=\"ParagraphCtrl\"][1]//div[@class=\"control\"]//span[1][text()=\" FINISHED \"]"}
```
---
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.
---
Re: [GitHub] incubator-zeppelin pull request: synchronization saving notebook.
Posted by Hyung Sung Shim <hs...@nflabs.com>.
Okay. I got it.
I'm going to pass CI.
Thank you Alexander.
2015-12-01 19:16 GMT+09:00 Alexander Bezzubov <bz...@apache.org>:
> Yes, I know and have the same problem with #497
>
> The usual way such problems to be handled though is not just merge it, but
> - either try re-trigger the CI to see if the is flanky test indeed (is not
> reproduced then)
> - or try to fix the test a new PR, merge it first
>
> and then get back to this again and try one more time.
>
> I know this is not easy, but that's the pattern we used so far to fight
> with "technical dept" - CI must be green for a merge.
>
> It also worth asking @Leemoonsoo @jongyoul for help in tricky cases. I will
> take a look too.
>
> On Tue, Dec 1, 2015, 18:53 astroshim <gi...@git.apache.org> wrote:
>
> > Github user astroshim commented on the pull request:
> >
> >
> >
> https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160916885
> >
> > @bzz Thank you very much!!
> > It seems like CI fails is not related to testcase. What do you think?
> >
> >
> > ---
> > 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.
> > ---
> >
>
Re: [GitHub] incubator-zeppelin pull request: synchronization saving notebook.
Posted by Alexander Bezzubov <bz...@apache.org>.
Yes, I know and have the same problem with #497
The usual way such problems to be handled though is not just merge it, but
- either try re-trigger the CI to see if the is flanky test indeed (is not
reproduced then)
- or try to fix the test a new PR, merge it first
and then get back to this again and try one more time.
I know this is not easy, but that's the pattern we used so far to fight
with "technical dept" - CI must be green for a merge.
It also worth asking @Leemoonsoo @jongyoul for help in tricky cases. I will
take a look too.
On Tue, Dec 1, 2015, 18:53 astroshim <gi...@git.apache.org> wrote:
> Github user astroshim commented on the pull request:
>
>
> https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160916885
>
> @bzz Thank you very much!!
> It seems like CI fails is not related to testcase. What do you think?
>
>
> ---
> 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: synchronization saving notebook.
Posted by astroshim <gi...@git.apache.org>.
Github user astroshim commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-160916885
@bzz Thank you very much!!
It seems like CI fails is not related to testcase. What do you think?
---
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: synchronization saving notebook.
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/489#issuecomment-161592950
Will merge, if there are no other 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: synchronization saving notebook.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-zeppelin/pull/489
---
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.
---