You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@livy.apache.org by aromanenko-dev <gi...@git.apache.org> on 2018/11/22 14:27:01 UTC
[GitHub] incubator-livy pull request #129: [LIVY-535] Fix non-atomic session creation
GitHub user aromanenko-dev opened a pull request:
https://github.com/apache/incubator-livy/pull/129
[LIVY-535] Fix non-atomic session creation
## What changes were proposed in this pull request?
All steps, that include check for too many sessions and creating new
session, should be atomic operation to avoid concurrent access of
several threads to different part of this code in the same time.
https://issues.apache.org/jira/browse/LIVY-535
## How was this patch tested?
If was tested manually on local virtual cluster with this different values of config option `livy.server.session.max-creation`: 1, 2 and 3
Then I run the following command to make sure that only allowed number of sessions were created in the same time and others were rejected:
`for i in {1..5}; do curl -X POST --data '{"file": "/tmp/spark-examples-2.jar", "className": "org.apache.spark.examples.SparkPi"}' -H "Content-Type: application/json" http://localhost:8999/batches; done`
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aromanenko-dev/incubator-livy LIVY-535-session-max-creation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-livy/pull/129.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 #129
----
commit a01659af8acc5a11ce815eefb591d41748fc0901
Author: Alexey Romanenko <ar...@...>
Date: 2018-11-22T14:03:55Z
[LIVY-535] Fix non-atomic session creation
All steps, that include check for too many sessions and creating new
session, should be atomic operation to avoid concurrent access of
several threads to different part of this code in the same time.
----
---
[GitHub] incubator-livy issue #129: [LIVY-535] Fix non-atomic session creation
Posted by Jassy1994 <gi...@git.apache.org>.
Github user Jassy1994 commented on the issue:
https://github.com/apache/incubator-livy/pull/129
using synchronize may be very slow when submitted many session concurrently, are there any other solutions to solve this problem with a smaller waiting time increase?
---
[GitHub] incubator-livy issue #129: [LIVY-535] Fix non-atomic session creation
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/incubator-livy/pull/129
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/129?src=pr&el=h1) Report
> Merging [#129](https://codecov.io/gh/apache/incubator-livy/pull/129?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/4cfb6bcb8fb9ac6b2d6c8b3d04b20f647b507e1f?src=pr&el=desc) will **increase** coverage by `0.11%`.
> The diff coverage is `87.5%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/129?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #129 +/- ##
============================================
+ Coverage 70.98% 71.09% +0.11%
- Complexity 925 927 +2
============================================
Files 100 100
Lines 5511 5512 +1
Branches 829 830 +1
============================================
+ Hits 3912 3919 +7
+ Misses 1058 1053 -5
+ Partials 541 540 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/129?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../scala/org/apache/livy/server/SessionServlet.scala](https://codecov.io/gh/apache/incubator-livy/pull/129/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvU2Vzc2lvblNlcnZsZXQuc2NhbGE=) | `67.07% <87.5%> (+0.4%)` | `16 <0> (ø)` | :arrow_down: |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/129/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
| [...c/main/scala/org/apache/livy/repl/ReplDriver.scala](https://codecov.io/gh/apache/incubator-livy/pull/129/diff?src=pr&el=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9SZXBsRHJpdmVyLnNjYWxh) | `30.76% <0%> (-2.57%)` | `7% <0%> (ø)` | |
| [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/129/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `78.87% <0%> (-0.71%)` | `33% <0%> (ø)` | |
| [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/129/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `61.11% <0%> (ø)` | `2% <0%> (ø)` | :arrow_down: |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/129/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `80% <0%> (ø)` | `8% <0%> (+1%)` | :arrow_up: |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/129/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `79.23% <0%> (+1.27%)` | `42% <0%> (+1%)` | :arrow_up: |
| [...c/src/main/java/org/apache/livy/rsc/RSCClient.java](https://codecov.io/gh/apache/incubator-livy/pull/129/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9SU0NDbGllbnQuamF2YQ==) | `83.85% <0%> (+3.72%)` | `26% <0%> (ø)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/129?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/129?src=pr&el=footer). Last update [4cfb6bc...a01659a](https://codecov.io/gh/apache/incubator-livy/pull/129?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
---
[GitHub] incubator-livy issue #129: [LIVY-535] Fix non-atomic session creation
Posted by aromanenko-dev <gi...@git.apache.org>.
Github user aromanenko-dev commented on the issue:
https://github.com/apache/incubator-livy/pull/129
@Jassy1994 I tend to agree with @vanzin - it was a simple fix for a bug in current implementation. Also, I kept in mind that this code won't take too much time, so the waiting time should be quite acceptable. Please, let me know if it's not so in your case.
---
[GitHub] incubator-livy issue #129: [LIVY-535] Fix non-atomic session creation
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/incubator-livy/pull/129
Took a look at the surrounding code and this seems fine. I was worried that `createSession` might be blocking, but it's not.
Merging to master.
---
[GitHub] incubator-livy issue #129: [LIVY-535] Fix non-atomic session creation
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/incubator-livy/pull/129
The code shouldn't be spending that much time in the synchronized block. It does do quite a lot of things, including spawning a child process, but it doesn't wait for that process to finish or anything like that. So it should be reasonably ok.
Otherwise this fix would be way more complicated. The accounting for how many things are starting is all over the place. If you want to spend time fixing that instead...
---
[GitHub] incubator-livy pull request #129: [LIVY-535] Fix non-atomic session creation
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-livy/pull/129
---