[LIVY-239] Generate session IDs in SessionStore, not in SessionManager by meisam · Pull Request #103 · apache/livy

meisam · 2018-07-17T00:31:56Z

What changes were proposed in this pull request?

Move the logic to create session IDs from SessionManager to SessionStore

Why this change is needed?

Generating session IDs at session stores that they are unique across all Livy instances that connect to a session store. Right now only one Livy instance connects to the session store, but to implement multimode HA, generating session IDs in session store prevents inconsistent/colliding values.

How was this patch tested?

Unit tests are added.
This patch is working in production at PayPal for about a year now.

JIRA ticket: https://issues.apache.org/jira/browse/LIVY-239

meisam · 2018-07-17T01:17:35Z

sudo -E apt-get ...

failed on one of the travis instances. Closing this PR and opening it again to rebuild.

codecov-io · 2018-07-17T03:48:05Z

Codecov Report

Merging #103 into master will decrease coverage by 0.11%.
The diff coverage is 61.53%.

@@             Coverage Diff              @@
##             master     #103      +/-   ##
============================================
- Coverage     68.79%   68.67%   -0.12%     
  Complexity      905      905              
============================================
  Files           100      100              
  Lines          5650     5651       +1     
  Branches        846      845       -1     
============================================
- Hits           3887     3881       -6     
- Misses         1211     1219       +8     
+ Partials        552      551       -1

Impacted Files	Coverage Δ	Complexity Δ
...a/org/apache/livy/server/recovery/StateStore.scala	`83.33% <ø> (ø)`	`1 <0> (ø)`	⬇️
...che/livy/server/recovery/ZooKeeperStateStore.scala	`70.83% <0%> (-8.24%)`	`17 <0> (ø)`
...he/livy/server/recovery/FileSystemStateStore.scala	`65.3% <100%> (+2.26%)`	`12 <1> (+1)`	⬆️
...org/apache/livy/server/recovery/SessionStore.scala	`82.35% <100%> (+2.35%)`	`9 <2> (-1)`	⬇️
...che/livy/server/recovery/BlackholeStateStore.scala	`100% <100%> (ø)`	`7 <2> (+2)`	⬆️
...cala/org/apache/livy/sessions/SessionManager.scala	`81.92% <100%> (-0.1%)`	`23 <1> (-1)`
...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala	`52.94% <0%> (-2.95%)`	`7% <0%> (ø)`
...c/main/scala/org/apache/livy/repl/ReplDriver.scala	`30.76% <0%> (-2.57%)`	`7% <0%> (ø)`
...c/src/main/java/org/apache/livy/rsc/RSCClient.java	`73.91% <0%> (-1.25%)`	`20% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80daade...3baf264. Read the comment docs.

change ket --> key

vanzin

Looks reasonable from what I remember of this code.

vanzin · 2018-08-22T18:48:01Z

+    *
+    * @return incremented value
+    */
+  def increment(key: String): Long


This method name is weird, since you're not incrementing the StateStore itself... maybe nextValue instead (or nextSessionId to follow the name in other parts of the code).

How about incrementAndGet() to emphasize two things:

It is an atomic operation.

It is not solely for generating session IDs.

My second choice is nextValue. But nextSessionId is a high level operation. It is not a good fit for StateStore whose other operations are get, set, remove, and getChildren.

alex-the-man · 2018-09-01T00:19:01Z

Can we add some migration code so user can seamlessly upgrade from older Livy? Thanks!

- Rename `StateStore.increment()` to `StateStore.nextValue()` - Change the path for storing nextSessionId to - `$STORE_VERSION/$sessionType/nextSessionId`. Task-url: https://issues.apache.org/jira/projects/LIVY/issues/LIVY-555

Task-url: https://issues.apache.org/jira/projects/LIVY/issues/LIVY-555

- Remove unused and unnecessary imports. Task-url: https://issues.apache.org/jira/projects/LIVY/issues/LIVY-555

- Properly mock SessionStore in test cases. Task-url: https://issues.apache.org/jira/projects/LIVY/issues/LIVY-555

meisam · 2019-02-08T01:27:28Z

Can we add some migration code so user can seamlessly upgrade from older Livy? Thanks!

Should the migration code be part of Livy? Or should be a separate script that users run once?

Also should we change the version in zookeeper path from /v1/... to /v2/...?

alex-the-man · 2019-02-14T00:47:31Z

Can we add some migration code so user can seamlessly upgrade from older Livy? Thanks!

Should the migration code be part of Livy? Or should be a separate script that users run once?

In this case it seems simple enough to be a part of the startup code.

Also should we change the version in zookeeper path from /v1/... to /v2/...?

I think you are right, we should.

yiheng · 2019-09-18T07:43:37Z

 */
 class BlackholeStateStore(livyConf: LivyConf) extends StateStore(livyConf) {
+
+  private val atomicLong: AtomicLong = new AtomicLong(-1L)


Looks like this cannot guarantee the global uniqueness across many Livy services, right? As the atomicLong cannot be shared by processes.

yiheng · 2019-09-18T07:45:07Z

  private def absPath(key: String): Path = new Path(fsUri.getPath(), key)
+
+  override def nextValue(key: String): Long = synchronized {
+    val incrementedValue = 1 + get[Long](key).getOrElse(-1L)


Is the get or set method atomic?

github-actions · 2026-07-04T00:50:36Z

This pull request has been automatically marked as stale because it has had no activity for at least 3 months. If you are still working on this change or plan to move it forward, please leave a comment or push a new commit so we know to keep it open. Otherwise, this PR will be closed automatically in about one month. Thank you for your contribution to Apache Livy!

LIVY-239: Generate session IDs in SessionStore, not in SessionManager

31f3bc4

meisam closed this Jul 17, 2018

meisam reopened this Jul 17, 2018

meisam closed this Jul 17, 2018

meisam reopened this Jul 17, 2018

meisam closed this Jul 17, 2018

meisam reopened this Jul 17, 2018

meisam added 2 commits July 17, 2018 13:17

[LIVY-239] Fix the typo in BlackholeStateStore

6bce007

change ket --> key

[LIVY-239] Test increment in blackhole state store

38880d8

vanzin reviewed Aug 22, 2018

View reviewed changes

alex-the-man reviewed Sep 1, 2018

View reviewed changes

Comment thread server/src/main/scala/org/apache/livy/server/recovery/SessionStore.scala Outdated

meisam mentioned this pull request Feb 5, 2019

[LIVY-41] Let users access sessions by session name #48

Closed

meisam added 4 commits February 7, 2019 13:13

[LIVY-555] Do not accept new sessions on Livy shutdown.

5e038e5

- Rename `StateStore.increment()` to `StateStore.nextValue()` - Change the path for storing nextSessionId to - `$STORE_VERSION/$sessionType/nextSessionId`. Task-url: https://issues.apache.org/jira/projects/LIVY/issues/LIVY-555

[LIVY-555] Merge with apache/master branch and resolve conflicts

004c8b9

Task-url: https://issues.apache.org/jira/projects/LIVY/issues/LIVY-555

[LIVY-555] Merge with apache/master branch and resolve conflicts

2490050

- Remove unused and unnecessary imports. Task-url: https://issues.apache.org/jira/projects/LIVY/issues/LIVY-555

[LIVY-555] Generated unique sessionIds in test cases

3baf264

- Properly mock SessionStore in test cases. Task-url: https://issues.apache.org/jira/projects/LIVY/issues/LIVY-555

yiheng reviewed Sep 18, 2019

View reviewed changes

github-actions Bot added the stale label Jul 4, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[LIVY-239] Generate session IDs in SessionStore, not in SessionManager#103

[LIVY-239] Generate session IDs in SessionStore, not in SessionManager#103
meisam wants to merge 7 commits into
apache:masterfrom
meisam:LIVY-239

meisam commented Jul 17, 2018 •

edited

Loading

Uh oh!

meisam commented Jul 17, 2018

Uh oh!

codecov-io commented Jul 17, 2018 •

edited

Loading

Uh oh!

vanzin left a comment

Uh oh!

vanzin Aug 22, 2018 •

edited

Loading

Uh oh!

meisam Jan 23, 2019 •

edited

Loading

Uh oh!

Uh oh!

alex-the-man commented Sep 1, 2018

Uh oh!

meisam commented Feb 8, 2019

Uh oh!

alex-the-man commented Feb 14, 2019 •

edited

Loading

Uh oh!

yiheng Sep 18, 2019

Uh oh!

yiheng Sep 18, 2019

Uh oh!

github-actions Bot commented Jul 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

meisam commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why this change is needed?

How was this patch tested?

Uh oh!

meisam commented Jul 17, 2018

Uh oh!

codecov-io commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vanzin left a comment

Choose a reason for hiding this comment

Uh oh!

vanzin Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meisam Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alex-the-man commented Sep 1, 2018

Uh oh!

meisam commented Feb 8, 2019

Uh oh!

alex-the-man commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yiheng Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

yiheng Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

meisam commented Jul 17, 2018 •

edited

Loading

codecov-io commented Jul 17, 2018 •

edited

Loading

vanzin Aug 22, 2018 •

edited

Loading

meisam Jan 23, 2019 •

edited

Loading

alex-the-man commented Feb 14, 2019 •

edited

Loading