You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by sanjaydasgupta <gi...@git.apache.org> on 2018/06/01 17:23:09 UTC
[GitHub] zeppelin pull request #3000: [ZEPPELIN-3467] two-step, atomic configuration ...
GitHub user sanjaydasgupta opened a pull request:
https://github.com/apache/zeppelin/pull/3000
[ZEPPELIN-3467] two-step, atomic configuration file
### What is this PR for?
This PR continues the discussion and implementation in [PR-2978](https://github.com/apache/zeppelin/pull/2978) which had to be closed earlier.
This PR prevents the total loss of configuration information that happens when zeppelin attempts to write out modified configuration information into an already full file-system. The approach taken here is to first write the modified information to a temporary file (in the same directory), and then to rename the successfully written file to its eventual name.
The rename operation is not attempted if there is an error while writing the temporary file, thus leaving the pre-existing configuration file untouched.
### What type of PR is it?
[Improvement]
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-3467
### How should this be tested?
CI Pass
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sanjaydasgupta/zeppelin zepp-3467-atomic-config-file-writes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/3000.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 #3000
----
commit be19e63283894e80e11174137405601cdbf08e53
Author: Sanjay Dasgupta <sa...@...>
Date: 2018-06-01T13:46:08Z
zepp-3467-atomic-config-file-writes: Initial updates
----
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3000
LGTM
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/3000
One minor comment, rest LGTM
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3000
@jongyoul the merge occurred by mistake, and I am not being able to revert it using the `git revert -m 1 <commit>` command.
I've tried looking up all help on the net, but am not getting anywhere. Please let me know if you have any ideas that may help.
I can create a clean new PR if that will be better.
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3000
@jongyoul please ignore my previous comments (now deleted)
The rebase has been done.
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3000
I've fixed the style issues pointed out.
One test still fails on travis--despite a rebase and restart. But the failure seems unrelated to the changes made in this PR.
---
[GitHub] zeppelin pull request #3000: [ZEPPELIN-3467] two-step, atomic configuration ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/3000
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3000
Hi @jongyoul one test is still red. I have restarted it 3 times, but it has failed with the same result each time. Please let me know if there is anything else to try.
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by Tagar <gi...@git.apache.org>.
Github user Tagar commented on the issue:
https://github.com/apache/zeppelin/pull/3000
> The orphaned file will not be automatically overwritten later because File.createTempFile(...) will always attempt to create a new filename that is unused in the directory.
That makes now. Thanks for the detailed response.
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3000
Cleaning the cache helped. Thanks for the hint @zjffdu
All green now @jongyoul
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/3000
@zjffdu
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3000
@Tagar thanks for your comments.
The thought here was to make the best of a bad situation. If and when the `delete()` fails, no recovery action is possible at the user level. So the only further action left is to notify the run-time-library that we are no longer interested in the file, and that the RTL should try to delete it if possible at some logical point in time (process exit).
The orphaned file will not be automatically overwritten later because `File.createTempFile(...)` will always attempt to create a new filename that is unused in the directory.
This is probably an obscure corner case without major negative consequences when handled either way. It is also likely impossible to test in a repeatable way.
My only motivation for this addition was to silence the Intellij IDE's warning that the return value of `delete()` had been ignored, but I think there are no technical errors in the overall logic.
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3000
ping @felixcheung
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3000
@sanjaydasgupta Could you please rebase this PR from the current master? merging master results as same but it's not human readable.
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3000
Thanks. BTW, please check your CI and restart the failed ones
https://travis-ci.org/sanjaydasgupta/zeppelin/builds/401742347
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3000
Agreed @zjffdu.
I also made two other (similar) changes. If the `tempFile.delete()` fails, then `tempFile.deleteOnExit()` is invoked to request the system to handle it if possible on exit.
Thanks.
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3000
Have fixed the style issues @felixcheung.
One of the tests is failing--despite rebase and restart--but appears to be unrelated to the code change.
---
[GitHub] zeppelin issue #3000: [ZEPPELIN-3467] two-step, atomic configuration file
Posted by zjffdu <gi...@git.apache.org>.
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/3000
@sanjaydasgupta If you see the following error in travis build. Please clean travis cache first, then retrigger the build.
```
[ERROR] error: error while loading <root>, error in opening zip file
[ERROR] error: scala.reflect.internal.MissingRequirementError: object scala.runtime in compiler mirror not found.
[ERROR] at scala.reflect.internal.MissingRequirementError$.signal(MissingRequirementError.scala:16)
```
---