You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@livy.apache.org by GitBox <gi...@apache.org> on 2020/01/10 00:59:14 UTC

[GitHub] [incubator-livy] runzhiwang opened a new pull request #274: [LIVY-718] Distributed session id generation

runzhiwang opened a new pull request #274: [LIVY-718] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274
 
 
   ## What changes were proposed in this pull request?
   
   1. When generate unique session id with multiple-active HA mode. First, get the distributed lock,
   Second, get the session id from filesystem or zookeeper. Third, increase the session id and save it in the filesystem or zookeeper. Forth, release the distributed lock.
   
   2. ZooKeeperManager provides the distributed lock to generate the distributed session id.
   
   ## How was this patch tested?
   
   Existed UT and IT.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r368277893
 
 

 ##########
 File path: conf/livy.conf.template
 ##########
 @@ -138,6 +151,8 @@
 
 # The dir in zk to store the data about session.
 # livy.server.recovery.zk-state-store.key-prefix = livy
+# The dir in zk to store distributed lock.
+# livy.server.zk.lock = livy/zk/lock
 
 Review comment:
   should it be `/livy/zk/lock`

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-718] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-718] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.05%`.
   > The diff coverage is `60.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.09%   -0.06%     
   - Complexity      959      968       +9     
   ============================================
     Files           104      107       +3     
     Lines          5946     5986      +40     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4076      +24     
   - Misses         1312     1327      +15     
   - Partials        582      583       +1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [.../livy/sessions/DistributedSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9EaXN0cmlidXRlZFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `100% <100%> (ø)` | `1 <1> (?)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.19% <100%> (+0.05%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/sessions/LocalSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9Mb2NhbFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `100% <100%> (ø)` | `4 <4> (?)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.88% <50%> (-0.6%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66% <55.55%> (-0.67%)` | `19 <2> (+2)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <81.25%> (-0.34%)` | `27 <3> (ø)` | |
   | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/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/274/diff?src=pr&el=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9SZXBsRHJpdmVyLnNjYWxh) | `30.76% <0%> (-2.57%)` | `7% <0%> (ø)` | |
   | ... and [5 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...f6f9776](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **increase** coverage by `0.17%`.
   > The diff coverage is `80%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   + Coverage     68.14%   68.31%   +0.17%     
   - Complexity      959      961       +2     
   ============================================
     Files           104      105       +1     
     Lines          5946     6000      +54     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4099      +47     
   - Misses         1312     1317       +5     
   - Partials        582      584       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `65.11% <0%> (-1.56%)` | `17 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.2% <100%> (+0.07%)` | `21 <0> (ø)` | :arrow_down: |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `73.33% <50%> (+0.91%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.93% <66.66%> (+1.45%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <78.94%> (-0.34%)` | `30 <3> (+3)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `89.65% <89.65%> (ø)` | `2 <2> (?)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `77.27% <91.66%> (-2.73%)` | `6 <5> (-4)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...04cf38a](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r375038829
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionManager.scala
 ##########
 @@ -145,7 +157,11 @@ class SessionManager[S <: Session, R <: RecoveryMetadata : ClassTag](
   }
 
   def shutdown(): Unit = {
-    val recoveryEnabled = livyConf.get(LivyConf.RECOVERY_MODE) != SESSION_RECOVERY_MODE_OFF
+    val haMode = Option(livyConf.get(LivyConf.HA_MODE))
+      .orElse(Option(livyConf.get(LivyConf.RECOVERY_MODE)))
+      .map(_.trim).orNull
 
 Review comment:
   Also here, duplicated code.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `39.86%`.
   > The diff coverage is `50%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master     #274       +/-   ##
   =============================================
   - Coverage     68.14%   28.27%   -39.87%     
   + Complexity      959      357      -602     
   =============================================
     Files           104      105        +1     
     Lines          5946     6001       +55     
     Branches        899      903        +4     
   =============================================
   - Hits           4052     1697     -2355     
   - Misses         1312     3960     +2648     
   + Partials        582      344      -238
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `56% <0%> (-5.23%)` | `9 <0> (-2)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `0% <0%> (-66.67%)` | `0 <0> (-17)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `0% <0%> (-100%)` | `0 <0> (-5)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `0% <0%> (-100%)` | `0 <0> (-8)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `95.26% <100%> (-0.88%)` | `18 <0> (-3)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.45% <25%> (-1.03%)` | `11 <0> (ø)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `40.74% <40.74%> (ø)` | `2 <2> (?)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `63.63% <57.14%> (-18.19%)` | `20 <2> (-7)` | |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `56.25% <66.66%> (-16.17%)` | `1 <0> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `68.18% <83.33%> (-11.82%)` | `6 <5> (-4)` | |
   | ... and [81 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...27e6acb](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366138255
 
 

 ##########
 File path: conf/livy.conf.template
 ##########
 @@ -138,6 +140,8 @@
 
 # The dir in zk to store the data about session.
 # livy.server.recovery.zk-state-store.key-prefix = livy
+# The dir related to zookeeper utility, such as: distributed lock, service discovery.
+# livy.server.zk.utility.dir-prefix = livy/zk-utility
 
 Review comment:
   Should we change it to below?
   ```
   livy.server.zk.prefix = livy/zk
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `39.86%`.
   > The diff coverage is `50%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master     #274       +/-   ##
   =============================================
   - Coverage     68.14%   28.27%   -39.87%     
   + Complexity      959      357      -602     
   =============================================
     Files           104      105        +1     
     Lines          5946     6001       +55     
     Branches        899      903        +4     
   =============================================
   - Hits           4052     1697     -2355     
   - Misses         1312     3960     +2648     
   + Partials        582      344      -238
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `56% <0%> (-5.23%)` | `9 <0> (-2)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `0% <0%> (-66.67%)` | `0 <0> (-17)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `0% <0%> (-100%)` | `0 <0> (-5)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `0% <0%> (-100%)` | `0 <0> (-8)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `95.26% <100%> (-0.88%)` | `18 <0> (-3)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.45% <25%> (-1.03%)` | `11 <0> (ø)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `40.74% <40.74%> (ø)` | `2 <2> (?)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `63.63% <57.14%> (-18.19%)` | `20 <2> (-7)` | |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `56.25% <66.66%> (-16.17%)` | `1 <0> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `68.18% <83.33%> (-11.82%)` | `6 <5> (-4)` | |
   | ... and [81 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...27e6acb](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r375038681
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+import java.util.concurrent.atomic.AtomicInteger
+
+import org.apache.curator.framework.recipes.locks.InterProcessSemaphoreMutex
+
+import org.apache.livy.server.recovery.{SessionStore, ZooKeeperManager}
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore) {
+
+  /**
+    * Get next session id, then increase next session id and save it in store.
+    */
+  def getNextSessionId(): Int
+
+  protected def getAndIncreaseId(): Int
+
+  protected def persist(id: Int): Unit = {
+    sessionStore.saveNextSessionId(sessionType, id)
+  }
+}
+
+class LocalSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore) extends SessionIdGenerator(sessionType, sessionStore) {
+
+  private val idCounter = new AtomicInteger(0)
+  idCounter.set(sessionStore.getNextSessionId(sessionType))
+
+  override def getNextSessionId(): Int = {
+    val result = getAndIncreaseId()
+    persist(result + 1)
+    result
+  }
+
+  override def getAndIncreaseId(): Int = {
+    idCounter.getAndIncrement()
+  }
+}
+
+class DistributedSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore,
+    zkManager: ZooKeeperManager,
+    mockLock: Option[InterProcessSemaphoreMutex] = None)
+  extends SessionIdGenerator(sessionType, sessionStore) {
+
+  require(sessionStore.getStore.isDistributed(),
+    "Choose a distributed store such as hdfs or zookeeper")
+
+  val distributedLock = mockLock.getOrElse(
+    zkManager.createLock(SessionStore.sessionIdLockPath(sessionType)))
+
+  override def getNextSessionId(): Int = {
+    distributedLock.acquire()
+    val result = getAndIncreaseId()
+    persist(result + 1)
+    distributedLock.release()
 
 Review comment:
   What if there's an exception thrown in the above code, are we going to lose the lock?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.02%`.
   > The diff coverage is `60.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.12%   -0.03%     
   - Complexity      959      966       +7     
   ============================================
     Files           104      107       +3     
     Lines          5946     5986      +40     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4078      +26     
   - Misses         1312     1325      +13     
   - Partials        582      583       +1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [.../livy/sessions/DistributedSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9EaXN0cmlidXRlZFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `100% <100%> (ø)` | `1 <1> (?)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.19% <100%> (+0.05%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/sessions/LocalSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9Mb2NhbFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `100% <100%> (ø)` | `4 <4> (?)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.33% <50%> (-0.15%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66% <55.55%> (-0.67%)` | `19 <2> (+2)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <81.25%> (-0.34%)` | `27 <3> (ø)` | |
   | [...c/main/scala/org/apache/livy/repl/ReplDriver.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9SZXBsRHJpdmVyLnNjYWxh) | `30.76% <0%> (-2.57%)` | `7% <0%> (ø)` | |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...a665acf](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r375037690
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/server/recovery/StateStore.scala
 ##########
 @@ -104,9 +108,13 @@ object StateStore extends Logging {
   }
 
   private[recovery] def pickStateStore(livyConf: LivyConf): Class[_] = {
-    livyConf.get(LivyConf.RECOVERY_MODE) match {
-      case SESSION_RECOVERY_MODE_OFF => classOf[BlackholeStateStore]
-      case SESSION_RECOVERY_MODE_RECOVERY =>
+    val haMode = Option(livyConf.get(LivyConf.HA_MODE)).
+      orElse(Option(livyConf.get(LivyConf.RECOVERY_MODE))).
+      map(_.trim).orNull
 
 Review comment:
   Move `.` to the beginning of the line. Also there's other duplicated code in `testRecovery`, can we merge them together?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366143789
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator {
+  /**
+    * Get next session id, then increase next session id and save it in store.
+    */
+  def getNextSessionId(): Int
 
 Review comment:
   getNextSessionId -> getAndIncrementSessionId ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r375038290
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+import java.util.concurrent.atomic.AtomicInteger
+
+import org.apache.curator.framework.recipes.locks.InterProcessSemaphoreMutex
+
+import org.apache.livy.server.recovery.{SessionStore, ZooKeeperManager}
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore) {
+
+  /**
+    * Get next session id, then increase next session id and save it in store.
+    */
+  def getNextSessionId(): Int
+
+  protected def getAndIncreaseId(): Int
+
+  protected def persist(id: Int): Unit = {
+    sessionStore.saveNextSessionId(sessionType, id)
+  }
+}
+
+class LocalSessionIdGenerator(
 
 Review comment:
   Would be better to rename to `InMemorySessionIdGenerator`.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r368260163
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/LivyConf.scala
 ##########
 @@ -227,6 +242,9 @@ object LivyConf {
   val RECOVERY_ZK_STATE_STORE_KEY_PREFIX =
     Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
 
+  // The dir in zk to store distributed lock.
+  val ZK_LOCK_DIR = Entry("livy.server.zk.lock", "livy/zk/lock")
 
 Review comment:
   Is it better to change to `livy.server.ha.zk-lock`? Since it is HA related.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang commented on issue #274: [LIVY-718] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang commented on issue #274: [LIVY-718] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572826072
 
 
   @jerryshao @yiheng Could you help review this PR ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.02%`.
   > The diff coverage is `60.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.12%   -0.03%     
   - Complexity      959      966       +7     
   ============================================
     Files           104      107       +3     
     Lines          5946     5986      +40     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4078      +26     
   - Misses         1312     1325      +13     
   - Partials        582      583       +1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [.../livy/sessions/DistributedSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9EaXN0cmlidXRlZFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `100% <100%> (ø)` | `1 <1> (?)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.19% <100%> (+0.05%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/sessions/LocalSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9Mb2NhbFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `100% <100%> (ø)` | `4 <4> (?)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.33% <50%> (-0.15%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66% <55.55%> (-0.67%)` | `19 <2> (+2)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <81.25%> (-0.34%)` | `27 <3> (ø)` | |
   | [...c/main/scala/org/apache/livy/repl/ReplDriver.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9SZXBsRHJpdmVyLnNjYWxh) | `30.76% <0%> (-2.57%)` | `7% <0%> (ø)` | |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...a665acf](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366142503
 
 

 ##########
 File path: conf/livy.conf.template
 ##########
 @@ -138,6 +140,8 @@
 
 # The dir in zk to store the data about session.
 # livy.server.recovery.zk-state-store.key-prefix = livy
+# The dir related to zookeeper utility, such as: distributed lock, service discovery.
+# livy.server.zk.utility.dir-prefix = livy/zk-utility
 
 Review comment:
   Will this be only used in distributed lock and service discovery? If so, I suggest using configurations like
   ```
   # The dir in zk to store distributed lock
   livy.server.zk.lock = livy/zk/lock
   # The dir in zk to store service information
   livy.server.zk.services = livy/zk/services

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **increase** coverage by `0.17%`.
   > The diff coverage is `81.92%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   + Coverage     68.14%   68.32%   +0.17%     
   - Complexity      959      961       +2     
   ============================================
     Files           104      105       +1     
     Lines          5946     5998      +52     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4098      +46     
   - Misses         1312     1316       +4     
   - Partials        582      584       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `65.11% <0%> (-1.56%)` | `17 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.2% <100%> (+0.07%)` | `21 <0> (ø)` | :arrow_down: |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `73.33% <50%> (+0.91%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.93% <66.66%> (+1.45%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <78.94%> (-0.34%)` | `30 <3> (+3)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `77.27% <91.66%> (-2.73%)` | `6 <5> (-4)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `96.29% <96.29%> (ø)` | `2 <2> (?)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...e4ebea1](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **increase** coverage by `0.16%`.
   > The diff coverage is `80.95%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   + Coverage     68.14%   68.31%   +0.16%     
   - Complexity      959      962       +3     
   ============================================
     Files           104      105       +1     
     Lines          5946     5999      +53     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4098      +46     
   - Misses         1312     1317       +5     
   - Partials        582      584       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `65.11% <0%> (-1.56%)` | `17 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.2% <100%> (+0.07%)` | `21 <0> (ø)` | :arrow_down: |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `73.33% <50%> (+0.91%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.49% <66.66%> (+1.01%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <78.94%> (-0.34%)` | `30 <3> (+3)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `77.27% <91.66%> (-2.73%)` | `6 <5> (-4)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `92.85% <92.85%> (ø)` | `2 <2> (?)` | |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...b946126](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366666905
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator {
+  /**
+    * Get next session id, then increase next session id and save it in store.
+    */
+  def getNextSessionId(): Int
+
+  /**
+    * recover next session id from store.
+    */
+  def recover(): Unit
 
 Review comment:
   @yiheng Good point.  Actually  it does not need `recover ` any more. For `LocalSessionIdGenerator`, it store session id in memory, so can not recover. For `DistributedSessionIdGenerator`,  it will get session id from store every time `getNextSessionId`, so it does not need recover when restart. I will remove it.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366144127
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator {
+  /**
+    * Get next session id, then increase next session id and save it in store.
 
 Review comment:
   remove `and save it in store` ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r379223901
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+import java.util.concurrent.atomic.AtomicInteger
+
+import org.apache.curator.framework.recipes.locks.InterProcessSemaphoreMutex
+
+import org.apache.livy.server.recovery.{SessionStore, ZooKeeperManager}
+import org.apache.livy.utils.LivyUncaughtException
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore) {
+
+  /**
+    * Get next session id, then increase next session id and save it in store.
+    */
+  def getNextSessionId(): Int
+
+  protected def getAndIncreaseId(): Int
+
+  protected def persist(id: Int): Unit = {
+    sessionStore.saveNextSessionId(sessionType, id)
+  }
+}
+
+class LocalSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore) extends SessionIdGenerator(sessionType, sessionStore) {
+
+  private val idCounter = new AtomicInteger(0)
+  idCounter.set(sessionStore.getNextSessionId(sessionType))
+
+  override def getNextSessionId(): Int = {
+    val result = getAndIncreaseId()
+    persist(result + 1)
+    result
+  }
+
+  override def getAndIncreaseId(): Int = {
+    idCounter.getAndIncrement()
+  }
+}
+
+class DistributedSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore,
+    zkManager: ZooKeeperManager,
+    mockLock: Option[InterProcessSemaphoreMutex] = None)
+  extends SessionIdGenerator(sessionType, sessionStore) {
+
+  require(sessionStore.getStore.isDistributed(),
+    "Choose a distributed store such as hdfs or zookeeper")
+
+  val distributedLock = mockLock.getOrElse(
+    zkManager.createLock(SessionStore.sessionIdLockPath(sessionType)))
+
+  override def getNextSessionId(): Int = {
+    distributedLock.acquire()
+    try {
+      val result = getAndIncreaseId()
+      persist(result + 1)
+      result
+    } catch {
+      case e: Exception => throw new LivyUncaughtException(e.getMessage)
+    } finally {
+      distributedLock.release()
 
 Review comment:
   Will here throw an exception again?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io commented on issue #274: [LIVY-718] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io commented on issue #274: [LIVY-718] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.05%`.
   > The diff coverage is `60.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.09%   -0.06%     
   - Complexity      959      968       +9     
   ============================================
     Files           104      107       +3     
     Lines          5946     5986      +40     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4076      +24     
   - Misses         1312     1327      +15     
   - Partials        582      583       +1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [.../livy/sessions/DistributedSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9EaXN0cmlidXRlZFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `100% <100%> (ø)` | `1 <1> (?)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.19% <100%> (+0.05%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/sessions/LocalSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9Mb2NhbFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `100% <100%> (ø)` | `4 <4> (?)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.88% <50%> (-0.6%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66% <55.55%> (-0.67%)` | `19 <2> (+2)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <81.25%> (-0.34%)` | `27 <3> (ø)` | |
   | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/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/274/diff?src=pr&el=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9SZXBsRHJpdmVyLnNjYWxh) | `30.76% <0%> (-2.57%)` | `7% <0%> (ø)` | |
   | ... and [5 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...f6f9776](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.02%`.
   > The diff coverage is `60.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.12%   -0.03%     
   - Complexity      959      966       +7     
   ============================================
     Files           104      107       +3     
     Lines          5946     5986      +40     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4078      +26     
   - Misses         1312     1325      +13     
   - Partials        582      583       +1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [.../livy/sessions/DistributedSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9EaXN0cmlidXRlZFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `100% <100%> (ø)` | `1 <1> (?)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.19% <100%> (+0.05%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/sessions/LocalSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9Mb2NhbFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `100% <100%> (ø)` | `4 <4> (?)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.33% <50%> (-0.15%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66% <55.55%> (-0.67%)` | `19 <2> (+2)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <81.25%> (-0.34%)` | `27 <3> (ø)` | |
   | [...c/main/scala/org/apache/livy/repl/ReplDriver.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9SZXBsRHJpdmVyLnNjYWxh) | `30.76% <0%> (-2.57%)` | `7% <0%> (ø)` | |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...a665acf](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on issue #274: [LIVY-718] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on issue #274: [LIVY-718] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572835729
 
 
   Seems like the JIRA number is not the correct one. LIVY-718 is an umbrella jira.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366143789
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator {
+  /**
+    * Get next session id, then increase next session id and save it in store.
+    */
+  def getNextSessionId(): Int
 
 Review comment:
   getNextSessionId -> getAndIncrementSessionId ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-718] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-718] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.05%`.
   > The diff coverage is `60.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.09%   -0.06%     
   - Complexity      959      968       +9     
   ============================================
     Files           104      107       +3     
     Lines          5946     5986      +40     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4076      +24     
   - Misses         1312     1327      +15     
   - Partials        582      583       +1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [.../livy/sessions/DistributedSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9EaXN0cmlidXRlZFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `100% <100%> (ø)` | `1 <1> (?)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.19% <100%> (+0.05%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/sessions/LocalSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9Mb2NhbFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `100% <100%> (ø)` | `4 <4> (?)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.88% <50%> (-0.6%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66% <55.55%> (-0.67%)` | `19 <2> (+2)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <81.25%> (-0.34%)` | `27 <3> (ø)` | |
   | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/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/274/diff?src=pr&el=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9SZXBsRHJpdmVyLnNjYWxh) | `30.76% <0%> (-2.57%)` | `7% <0%> (ø)` | |
   | ... and [5 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...f6f9776](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366140466
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/DistributedSessionIdGenerator.scala
 ##########
 @@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+import org.apache.livy.server.recovery.BlackholeStateStore
+import org.apache.livy.server.recovery.SessionStore
+import org.apache.livy.server.recovery.ZooKeeperManager
+
+class DistributedSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore,
+    zkManager: ZooKeeperManager) extends SessionIdGenerator {
+
+  require(sessionStore.getStore.isInstanceOf[BlackholeStateStore] == false)
 
 Review comment:
   I don't think this check is safe. Say in the future a LocalFileSystem state store is added, the check will not alarm

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r368260986
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionManager.scala
 ##########
 @@ -145,7 +154,11 @@ class SessionManager[S <: Session, R <: RecoveryMetadata : ClassTag](
   }
 
   def shutdown(): Unit = {
-    val recoveryEnabled = livyConf.get(LivyConf.RECOVERY_MODE) != SESSION_RECOVERY_MODE_OFF
+    val haMode = Option(livyConf.get(LivyConf.HA_MODE)).
+      orElse(Option(livyConf.get(LivyConf.RECOVERY_MODE))).
+      map(_.trim).orNull
 
 Review comment:
   Nit: put `.` in the beginning of the line.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.08%`.
   > The diff coverage is `61.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.06%   -0.09%     
   - Complexity      959      962       +3     
   ============================================
     Files           104      105       +1     
     Lines          5946     5999      +53     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4083      +31     
   - Misses         1312     1331      +19     
   - Partials        582      585       +3
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.17% <100%> (+0.03%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `64.58% <42.85%> (-2.09%)` | `17 <0> (ø)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `50% <50%> (ø)` | `2 <2> (?)` | |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `75% <66.66%> (+2.58%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `35.06% <72.72%> (+1.58%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `80.55% <73.68%> (-1.27%)` | `27 <2> (ø)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | ... and [5 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...caf8ac0](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366135479
 
 

 ##########
 File path: conf/livy.conf.template
 ##########
 @@ -109,6 +109,8 @@
 # livy.server.recovery.mode = off
 # Zookeeper address used for HA and state store. e.g. host1:port1, host2:port2
 # livy.server.zookeeper.url =
+# Whether to enable HA with multi-active mode, by default it is false.
+# livy.server.ha.multi-active.enabled = false
 
 Review comment:
   Should we change it to something like?
   ```
   livy.servver.ha.mode=multi-active
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **increase** coverage by `0.17%`.
   > The diff coverage is `80%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   + Coverage     68.14%   68.31%   +0.17%     
   - Complexity      959      961       +2     
   ============================================
     Files           104      105       +1     
     Lines          5946     6000      +54     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4099      +47     
   - Misses         1312     1317       +5     
   - Partials        582      584       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `65.11% <0%> (-1.56%)` | `17 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.2% <100%> (+0.07%)` | `21 <0> (ø)` | :arrow_down: |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `73.33% <50%> (+0.91%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.93% <66.66%> (+1.45%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <78.94%> (-0.34%)` | `30 <3> (+3)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `89.65% <89.65%> (ø)` | `2 <2> (?)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `77.27% <91.66%> (-2.73%)` | `6 <5> (-4)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...04cf38a](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang closed pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang closed pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] wypoon commented on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

wypoon commented on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-580494955
 
 
   See https://github.com/apache/incubator-livy/pull/275/commits/29114951c48d3d1829d4a0fdaaa338983e193caf for my fix to the python-api CI failure.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r368461675
 
 

 ##########
 File path: conf/livy.conf.template
 ##########
 @@ -138,6 +151,8 @@
 
 # The dir in zk to store the data about session.
 # livy.server.recovery.zk-state-store.key-prefix = livy
+# The dir in zk to store distributed lock.
+# livy.server.zk.lock = livy/zk/lock
 
 Review comment:
   Do we really need a lock dir? I think the lock should be put on some node where the concurrent operation happens. For example, the session id file when generating a unique distributed session id.
   
   If we use a single lock dir, there will be some problem when the lock object is different. For example, generate session-id should not be blocked by update some session map.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.02%`.
   > The diff coverage is `60.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.12%   -0.03%     
   - Complexity      959      966       +7     
   ============================================
     Files           104      107       +3     
     Lines          5946     5986      +40     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4078      +26     
   - Misses         1312     1325      +13     
   - Partials        582      583       +1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [.../livy/sessions/DistributedSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9EaXN0cmlidXRlZFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `100% <100%> (ø)` | `1 <1> (?)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.19% <100%> (+0.05%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/sessions/LocalSessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9Mb2NhbFNlc3Npb25JZEdlbmVyYXRvci5zY2FsYQ==) | `100% <100%> (ø)` | `4 <4> (?)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.33% <50%> (-0.15%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66% <55.55%> (-0.67%)` | `19 <2> (+2)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <81.25%> (-0.34%)` | `27 <3> (ø)` | |
   | [...c/main/scala/org/apache/livy/repl/ReplDriver.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9SZXBsRHJpdmVyLnNjYWxh) | `30.76% <0%> (-2.57%)` | `7% <0%> (ø)` | |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...a665acf](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang commented on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang commented on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-580575713
 
 
   @jerryshao @yiheng UTs has been added.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang commented on issue #274: [LIVY-721] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang commented on issue #274: [LIVY-721] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572837580
 
 
   @jerryshao Yes, it's a wrong jira number. I have changed it from LIVY-718 to LIVY-721.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang opened a new pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang opened a new pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274
 
 
   ## What changes were proposed in this pull request?
   
   1. When generate unique session id with multiple-active HA mode. First, get the distributed lock,
   Second, get the session id from filesystem or zookeeper. Third, increase the session id and save it in the filesystem or zookeeper. Forth, release the distributed lock.
   
   2. ZooKeeperManager provides the distributed lock to generate the distributed session id.
   
   3. `LocalSessionIdGenerator` used when only one livy work. and `DistributedSessionIdGenerator` used when serveral livy work at the same time, such as in multi-active HA mode. Maybe the name `Local `of  `LocalSessionIdGenerator` is confused, because it also use hdfs or zookeeper. But I do not have a better name, maybe rename `LocalSessionIdGenerator` to `StandaloneSessionIdGenerator`  ？
     
   ## How was this patch tested?
   
   Existed UT and IT.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r368260894
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionManager.scala
 ##########
 @@ -84,13 +87,19 @@ class SessionManager[S <: Session, R <: RecoveryMetadata : ClassTag](
   private[this] final val sessionStateRetainedInSec =
     TimeUnit.MILLISECONDS.toNanos(livyConf.getTimeAsMs(LivyConf.SESSION_STATE_RETAIN_TIME))
 
+  private final val sessionIdGenerator = {
+    if (livyConf.get(LivyConf.HA_MODE) == LivyServer.HA_MODE_MULTI_ACTIVE) {
+      new DistributedSessionIdGenerator(sessionType, sessionStore, zkManager.get)
 
 Review comment:
   What if here `zkManager` is `None`?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366135479
 
 

 ##########
 File path: conf/livy.conf.template
 ##########
 @@ -109,6 +109,8 @@
 # livy.server.recovery.mode = off
 # Zookeeper address used for HA and state store. e.g. host1:port1, host2:port2
 # livy.server.zookeeper.url =
+# Whether to enable HA with multi-active mode, by default it is false.
+# livy.server.ha.multi-active.enabled = false
 
 Review comment:
   Should we change it to something like?
   ```
   livy.servver.ha.mode=multi-active
   ```
   
   should the `livy.server.recovery.mode` be **off** when we enable multi-active ha?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r368277893
 
 

 ##########
 File path: conf/livy.conf.template
 ##########
 @@ -138,6 +151,8 @@
 
 # The dir in zk to store the data about session.
 # livy.server.recovery.zk-state-store.key-prefix = livy
+# The dir in zk to store distributed lock.
+# livy.server.zk.lock = livy/zk/lock
 
 Review comment:
   should it be `/livy/zk/lock`

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **increase** coverage by `0.17%`.
   > The diff coverage is `81.92%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   + Coverage     68.14%   68.32%   +0.17%     
   - Complexity      959      961       +2     
   ============================================
     Files           104      105       +1     
     Lines          5946     5998      +52     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4098      +46     
   - Misses         1312     1316       +4     
   - Partials        582      584       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `65.11% <0%> (-1.56%)` | `17 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.2% <100%> (+0.07%)` | `21 <0> (ø)` | :arrow_down: |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `73.33% <50%> (+0.91%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.93% <66.66%> (+1.45%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <78.94%> (-0.34%)` | `30 <3> (+3)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `77.27% <91.66%> (-2.73%)` | `6 <5> (-4)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `96.29% <96.29%> (ø)` | `2 <2> (?)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...e4ebea1](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **increase** coverage by `0.14%`.
   > The diff coverage is `80.95%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   + Coverage     68.14%   68.29%   +0.14%     
   - Complexity      959      962       +3     
   ============================================
     Files           104      105       +1     
     Lines          5946     5999      +53     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4097      +45     
   - Misses         1312     1318       +6     
   - Partials        582      584       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `65.11% <0%> (-1.56%)` | `17 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.2% <100%> (+0.07%)` | `21 <0> (ø)` | :arrow_down: |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `73.33% <50%> (+0.91%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.06% <66.66%> (+0.57%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <78.94%> (-0.34%)` | `30 <3> (+3)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `77.27% <91.66%> (-2.73%)` | `6 <5> (-4)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `92.85% <92.85%> (ø)` | `2 <2> (?)` | |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...b946126](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **increase** coverage by `0.17%`.
   > The diff coverage is `80%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   + Coverage     68.14%   68.31%   +0.17%     
   - Complexity      959      961       +2     
   ============================================
     Files           104      105       +1     
     Lines          5946     6000      +54     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4099      +47     
   - Misses         1312     1317       +5     
   - Partials        582      584       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `65.11% <0%> (-1.56%)` | `17 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.2% <100%> (+0.07%)` | `21 <0> (ø)` | :arrow_down: |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `73.33% <50%> (+0.91%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.93% <66.66%> (+1.45%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <78.94%> (-0.34%)` | `30 <3> (+3)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `89.65% <89.65%> (ø)` | `2 <2> (?)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `77.27% <91.66%> (-2.73%)` | `6 <5> (-4)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...04cf38a](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang opened a new pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang opened a new pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274
 
 
   ## What changes were proposed in this pull request?
   
   1. When generate unique session id with multiple-active HA mode. First, get the distributed lock,
   Second, get the session id from filesystem or zookeeper. Third, increase the session id and save it in the filesystem or zookeeper. Forth, release the distributed lock.
   
   2. ZooKeeperManager provides the distributed lock to generate the distributed session id.
   
   3. `LocalSessionIdGenerator` used when only one livy work. and `DistributedSessionIdGenerator` used when serveral livy work at the same time, such as in multi-active HA mode. Maybe the name `Local `of  `LocalSessionIdGenerator` is confused, because it also use hdfs or zookeeper. But I do not have a better name, maybe rename `LocalSessionIdGenerator` to `StandaloneSessionIdGenerator`  ？
     
   ## How was this patch tested?
   
   Existed UT and IT.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366186498
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator {
+  /**
+    * Get next session id, then increase next session id and save it in store.
 
 Review comment:
   I think the `save it in store` is not guaranteed here. If we require the developer to follow the pattern, I suggest we break the getNextSessionId to two methods:
   
   ```scala
   /**
    * Get and increase the next session-id in an atomic operation. The session id will be persisted and recoverable.
    */
   def getNextSessionId: Int = {
       val nextSessionId = getAndIncreaseId()
       persist()
       return nextSessionId;
   }
   
   private def getAndIncreaseId: Int
   
   private def persist: Unit
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r368260330
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/server/LivyServer.scala
 ##########
 @@ -417,13 +417,20 @@ class LivyServer extends Logging {
   private[livy] def testRecovery(livyConf: LivyConf): Unit = {
     if (!livyConf.isRunningOnYarn()) {
       // If recovery is turned on but we are not running on YARN, quit.
-      require(livyConf.get(LivyConf.RECOVERY_MODE) == SESSION_RECOVERY_MODE_OFF,
+      val haMode = Option(livyConf.get(LivyConf.HA_MODE)).
+        orElse(Option(livyConf.get(LivyConf.RECOVERY_MODE))).
+        map(_.trim).orNull
 
 Review comment:
   nit: put `.` in the beginning of the line.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366143459
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator {
+  /**
+    * Get next session id, then increase next session id and save it in store.
+    */
+  def getNextSessionId(): Int
+
+  /**
+    * recover next session id from store.
+    */
+  def recover(): Unit
 
 Review comment:
   Do we really need the method to be called explicit? Can we hide it in the constructor?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366138255
 
 

 ##########
 File path: conf/livy.conf.template
 ##########
 @@ -138,6 +140,8 @@
 
 # The dir in zk to store the data about session.
 # livy.server.recovery.zk-state-store.key-prefix = livy
+# The dir related to zookeeper utility, such as: distributed lock, service discovery.
+# livy.server.zk.utility.dir-prefix = livy/zk-utility
 
 Review comment:
   Should we change it to below?
   ```
   livy.server.zk.prefix = livy/zk
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **increase** coverage by `0.17%`.
   > The diff coverage is `80%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   + Coverage     68.14%   68.31%   +0.17%     
   - Complexity      959      961       +2     
   ============================================
     Files           104      105       +1     
     Lines          5946     6000      +54     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4099      +47     
   - Misses         1312     1317       +5     
   - Partials        582      584       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `65.11% <0%> (-1.56%)` | `17 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.2% <100%> (+0.07%)` | `21 <0> (ø)` | :arrow_down: |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `73.33% <50%> (+0.91%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.93% <66.66%> (+1.45%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <78.94%> (-0.34%)` | `30 <3> (+3)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `89.65% <89.65%> (ø)` | `2 <2> (?)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `77.27% <91.66%> (-2.73%)` | `6 <5> (-4)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...04cf38a](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r379223075
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+import java.util.concurrent.atomic.AtomicInteger
+
+import org.apache.curator.framework.recipes.locks.InterProcessSemaphoreMutex
+
+import org.apache.livy.server.recovery.{SessionStore, ZooKeeperManager}
+import org.apache.livy.utils.LivyUncaughtException
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore) {
+
+  /**
+    * Get next session id, then increase next session id and save it in store.
+    */
+  def getNextSessionId(): Int
+
+  protected def getAndIncreaseId(): Int
+
+  protected def persist(id: Int): Unit = {
+    sessionStore.saveNextSessionId(sessionType, id)
+  }
+}
+
+class LocalSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore) extends SessionIdGenerator(sessionType, sessionStore) {
+
+  private val idCounter = new AtomicInteger(0)
+  idCounter.set(sessionStore.getNextSessionId(sessionType))
+
+  override def getNextSessionId(): Int = {
+    val result = getAndIncreaseId()
+    persist(result + 1)
+    result
+  }
+
+  override def getAndIncreaseId(): Int = {
+    idCounter.getAndIncrement()
+  }
+}
+
+class DistributedSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore,
+    zkManager: ZooKeeperManager,
+    mockLock: Option[InterProcessSemaphoreMutex] = None)
+  extends SessionIdGenerator(sessionType, sessionStore) {
+
+  require(sessionStore.getStore.isDistributed(),
+    "Choose a distributed store such as hdfs or zookeeper")
+
+  val distributedLock = mockLock.getOrElse(
+    zkManager.createLock(SessionStore.sessionIdLockPath(sessionType)))
+
+  override def getNextSessionId(): Int = {
+    distributedLock.acquire()
+    try {
+      val result = getAndIncreaseId()
+      persist(result + 1)
+      result
+    } catch {
+      case e: Exception => throw new LivyUncaughtException(e.getMessage)
 
 Review comment:
   What will be happened here when occurring an exception？

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366666905
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator {
+  /**
+    * Get next session id, then increase next session id and save it in store.
+    */
+  def getNextSessionId(): Int
+
+  /**
+    * recover next session id from store.
+    */
+  def recover(): Unit
 
 Review comment:
   @yiheng Good point.  Actually  it does not need `recover ` any more. For `LocalSessionIdGenerator`, it store session id in memory, so can not recover. For `DistributedSessionIdGenerator`,  it will get session id from store every time `getNextSessionId`, so it does not need recover when restart. I will remove it.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao closed pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao closed pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `39.86%`.
   > The diff coverage is `81.39%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master     #274       +/-   ##
   =============================================
   - Coverage     68.14%   28.27%   -39.87%     
   + Complexity      959      357      -602     
   =============================================
     Files           104      105        +1     
     Lines          5946     6001       +55     
     Branches        899      903        +4     
   =============================================
   - Hits           4052     1697     -2355     
   - Misses         1312     3960     +2648     
   + Partials        582      344      -238
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `56% <0%> (-5.23%)` | `9 <0> (-2)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `0% <0%> (-66.67%)` | `0 <0> (-17)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `0% <0%> (-100%)` | `0 <0> (-8)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `95.26% <100%> (-0.88%)` | `18 <0> (-3)` | |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.45% <62.5%> (-1.03%)` | `11 <0> (ø)` | |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `56.25% <66.66%> (-16.17%)` | `1 <0> (ø)` | |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `63.63% <76.19%> (-18.19%)` | `20 <3> (-7)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `0% <80%> (-100%)` | `0 <4> (-5)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `68.18% <91.66%> (-11.82%)` | `6 <5> (-4)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `40.74% <96.29%> (ø)` | `2 <2> (?)` | |
   | ... and [5 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...27e6acb](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.08%`.
   > The diff coverage is `61.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.06%   -0.09%     
   - Complexity      959      962       +3     
   ============================================
     Files           104      105       +1     
     Lines          5946     5999      +53     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4083      +31     
   - Misses         1312     1331      +19     
   - Partials        582      585       +3
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.17% <100%> (+0.03%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `64.58% <42.85%> (-2.09%)` | `17 <0> (ø)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `50% <50%> (ø)` | `2 <2> (?)` | |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `75% <66.66%> (+2.58%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `35.06% <72.72%> (+1.58%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `80.55% <73.68%> (-1.27%)` | `27 <2> (ø)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | ... and [5 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...caf8ac0](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366140466
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/DistributedSessionIdGenerator.scala
 ##########
 @@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+import org.apache.livy.server.recovery.BlackholeStateStore
+import org.apache.livy.server.recovery.SessionStore
+import org.apache.livy.server.recovery.ZooKeeperManager
+
+class DistributedSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore,
+    zkManager: ZooKeeperManager) extends SessionIdGenerator {
+
+  require(sessionStore.getStore.isInstanceOf[BlackholeStateStore] == false)
 
 Review comment:
   I don't think this check is safe. Say in the future a LocalFileSystem state store is added, the check will not alarm.
   
   BTW, it's better to add message for require method.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **decrease** coverage by `0.08%`.
   > The diff coverage is `61.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   - Coverage     68.14%   68.06%   -0.09%     
   - Complexity      959      962       +3     
   ============================================
     Files           104      105       +1     
     Lines          5946     5999      +53     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4083      +31     
   - Misses         1312     1331      +19     
   - Partials        582      585       +3
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `76.19% <0%> (-3.81%)` | `10 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.17% <100%> (+0.03%)` | `21 <0> (ø)` | :arrow_down: |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `64.58% <42.85%> (-2.09%)` | `17 <0> (ø)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `50% <50%> (ø)` | `2 <2> (?)` | |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `75% <66.66%> (+2.58%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `35.06% <72.72%> (+1.58%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `80.55% <73.68%> (-1.27%)` | `27 <2> (ø)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | ... and [5 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...caf8ac0](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366142784
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/DistributedSessionIdGenerator.scala
 ##########
 @@ -0,0 +1,40 @@
+/*
 
 Review comment:
   I think you can merge DistributedSessionIdGenerator, LocalSessionIdGenerator and SessionIdGenerator to one file

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366140466
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/DistributedSessionIdGenerator.scala
 ##########
 @@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+import org.apache.livy.server.recovery.BlackholeStateStore
+import org.apache.livy.server.recovery.SessionStore
+import org.apache.livy.server.recovery.ZooKeeperManager
+
+class DistributedSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore,
+    zkManager: ZooKeeperManager) extends SessionIdGenerator {
+
+  require(sessionStore.getStore.isInstanceOf[BlackholeStateStore] == false)
 
 Review comment:
   I don't think this check is safe. Say in the future a LocalFileSystem state store is added, the check will not alarm.
   
   I suggest adding an `isDistributed` method to `StateStore` to indicate whether the state store can be accessed distribute, and check that method here. The state store developer make sure the method be overwritten correctly.
   
   BTW, it's better to add message for require method.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366143576
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/LocalSessionIdGenerator.scala
 ##########
 @@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+import java.util.concurrent.atomic.AtomicInteger
+
+import org.apache.livy.server.recovery.SessionStore
+
+class LocalSessionIdGenerator(
+    sessionType: String,
+    sessionStore: SessionStore) extends SessionIdGenerator {
+
+  private val idCounter = new AtomicInteger(0)
+
+  def getNextSessionId(): Int = {
+    val result = idCounter.getAndIncrement()
+    sessionStore.saveNextSessionId(sessionType, result + 1)
+    result
+  }
+
+  def recover(): Unit = {
+    idCounter.set(sessionStore.getNextSessionId(sessionType))
 
 Review comment:
   can we move it to constructor?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-574045258
 
 
   Do we have some test cases to cover the sessionIdGenerator?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r368269899
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/server/LivyServer.scala
 ##########
 @@ -417,13 +417,20 @@ class LivyServer extends Logging {
   private[livy] def testRecovery(livyConf: LivyConf): Unit = {
     if (!livyConf.isRunningOnYarn()) {
       // If recovery is turned on but we are not running on YARN, quit.
-      require(livyConf.get(LivyConf.RECOVERY_MODE) == SESSION_RECOVERY_MODE_OFF,
+      val haMode = Option(livyConf.get(LivyConf.HA_MODE)).
+        orElse(Option(livyConf.get(LivyConf.RECOVERY_MODE))).
+        map(_.trim).orNull
+
+      require(haMode == LivyServer.HA_MODE_OFF,
         "Session recovery requires YARN.")
     }
   }
 }
 
 object LivyServer {
+  val HA_MODE_OFF = "off"
 
 Review comment:
   move these to LivyConf ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

jerryshao commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r375038954
 
 

 ##########
 File path: server/src/test/scala/org/apache/livy/sessions/SessionManagerSpec.scala
 ##########
 @@ -271,17 +272,61 @@ class SessionManagerSpec extends FunSpec with Matchers with LivyBaseUnitTestSuit
 
     it("should not delete sessions on shutdown with recovery is on") {
       val conf = new LivyConf()
-      conf.set(LivyConf.RECOVERY_MODE, SessionManager.SESSION_RECOVERY_MODE_RECOVERY)
+      conf.set(LivyConf.RECOVERY_MODE, LivyConf.HA_MODE_RECOVERY)
 
       val sessionId = 24
       val sessionStore = mock[SessionStore]
       val session = mockSession(sessionId)
 
-      val sm = new BatchSessionManager(conf, sessionStore, Some(Seq(session)))
+      val sm = new BatchSessionManager(conf, sessionStore, None, Some(Seq(session)))
       sm.get(sessionId) shouldBe defined
       sm.shutdown()
 
       verify(session, never).stop()
     }
+
+    it("should crate instance of LocalSessionIdGenerator if livy.server.ha.mode is off") {
 
 Review comment:
   Typo: "create".

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-572834042
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=h1) Report
   > Merging [#274](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/66b5833e413bc10e39e3b92b585f496444c147d4?src=pr&el=desc) will **increase** coverage by `0.16%`.
   > The diff coverage is `80.95%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/274/graphs/tree.svg?width=650&token=0MkVbiUFwE&height=150&src=pr)](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #274      +/-   ##
   ============================================
   + Coverage     68.14%   68.31%   +0.16%     
   - Complexity      959      962       +3     
   ============================================
     Files           104      105       +1     
     Lines          5946     5999      +53     
     Branches        899      902       +3     
   ============================================
   + Hits           4052     4098      +46     
   - Misses         1312     1317       +5     
   - Partials        582      584       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `60% <0%> (-1.23%)` | `11 <0> (ø)` | |
   | [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `65.11% <0%> (-1.56%)` | `17 <0> (ø)` | |
   | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `90% <0%> (-10%)` | `8 <0> (ø)` | |
   | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.2% <100%> (+0.07%)` | `21 <0> (ø)` | :arrow_down: |
   | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `73.33% <50%> (+0.91%)` | `1 <0> (ø)` | :arrow_down: |
   | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.49% <66.66%> (+1.01%)` | `11 <0> (ø)` | :arrow_down: |
   | [...cala/org/apache/livy/sessions/SessionManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uTWFuYWdlci5zY2FsYQ==) | `81.48% <78.94%> (-0.34%)` | `30 <3> (+3)` | |
   | [...che/livy/server/recovery/BlackholeStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvQmxhY2tob2xlU3RhdGVTdG9yZS5zY2FsYQ==) | `83.33% <80%> (-16.67%)` | `5 <4> (ø)` | |
   | [...org/apache/livy/server/recovery/SessionStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU2Vzc2lvblN0b3JlLnNjYWxh) | `77.27% <91.66%> (-2.73%)` | `6 <5> (-4)` | |
   | [.../org/apache/livy/sessions/SessionIdGenerator.scala](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXNzaW9ucy9TZXNzaW9uSWRHZW5lcmF0b3Iuc2NhbGE=) | `92.85% <92.85%> (ø)` | `2 <2> (?)` | |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/274/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/274?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/274?src=pr&el=footer). Last update [66b5833...b946126](https://codecov.io/gh/apache/incubator-livy/pull/274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366674450
 
 

 ##########
 File path: conf/livy.conf.template
 ##########
 @@ -138,6 +140,8 @@
 
 # The dir in zk to store the data about session.
 # livy.server.recovery.zk-state-store.key-prefix = livy
+# The dir related to zookeeper utility, such as: distributed lock, service discovery.
+# livy.server.zk.utility.dir-prefix = livy/zk-utility
 
 Review comment:
   In this PR, it only used for  distributed lock. And I will change it to `livy.server.zk.lock = livy/zk/lock`. `livy.server.zk.services = livy/zk/services` will be added in the PR used it.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r366144127
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/SessionIdGenerator.scala
 ##########
 @@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.sessions
+
+/**
+  * Interface of session id generator.
+  */
+abstract class SessionIdGenerator {
+  /**
+    * Get next session id, then increase next session id and save it in store.
 
 Review comment:
   remove `and save it in store` ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

yiheng commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r368273741
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/LivyConf.scala
 ##########
 @@ -186,6 +189,18 @@ object LivyConf {
    */
   val RECOVERY_MODE = Entry("livy.server.recovery.mode", "off")
 
+  /**
+    * HA mode of Livy. Possible values:
+    * 1. null: Default. livy will use the value of livy.server.recovery.mode
+    * 2. off: Turn off recovery. Every time Livy shuts down, it stops and forgets all sessions.
+    * 3. recovery: Livy persists session info to the state store. When Livy restarts, it recovers
+    *              previous sessions from the state store.
+    *              Must set livy.server.recovery.state-store and
+    *              livy.server.recovery.state-store.url to configure the state store.
+    * 4. multi-active: HA with multi-active mode.
+    */
+  val HA_MODE = Entry("livy.server.ha.mode", null)
 
 Review comment:
   default value is off?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

runzhiwang commented on a change in pull request #274: [LIVY-721][SERVER] Distributed session id generation
URL: https://github.com/apache/incubator-livy/pull/274#discussion_r372724899
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/LivyConf.scala
 ##########
 @@ -186,6 +189,18 @@ object LivyConf {
    */
   val RECOVERY_MODE = Entry("livy.server.recovery.mode", "off")
 
+  /**
+    * HA mode of Livy. Possible values:
+    * 1. null: Default. livy will use the value of livy.server.recovery.mode
+    * 2. off: Turn off recovery. Every time Livy shuts down, it stops and forgets all sessions.
+    * 3. recovery: Livy persists session info to the state store. When Livy restarts, it recovers
+    *              previous sessions from the state store.
+    *              Must set livy.server.recovery.state-store and
+    *              livy.server.recovery.state-store.url to configure the state store.
+    * 4. multi-active: HA with multi-active mode.
+    */
+  val HA_MODE = Entry("livy.server.ha.mode", null)
 
 Review comment:
   If default value is off, Livy will use `livy.server.ha.mode` rather than` livy.server.recovery.mode` even though  user does not config livy.server.ha.mode, which break back-compatibility.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-livy] andrasbeni commented on pull request #274: [LIVY-721][SERVER] Distributed session id generation

Posted by GitBox <gi...@apache.org>.

andrasbeni commented on pull request #274:
URL: https://github.com/apache/incubator-livy/pull/274#issuecomment-651220914


   @runzhiwang do you plan to continue working on this? If not, I am happy to address present concerns and move forward the feature.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org