LIVY-239: Moving the logic to generate session IDs from Session Manager to SessionStore #220

meisam · 2016-11-03T00:31:26Z

Summary of changes:

Removes the AtomicCounter that is used in SessionManager to generate session IDs.
Generates the session IDs in SessionStore instead of SessionManager
Writes the newly generated session ID to the store each time nextSessionId is called.

Task-url: https://issues.cloudera.org/browse/LIVY-239

codecov-io · 2016-11-03T01:32:11Z

Current coverage is 71.78% (diff: 54.54%)

Merging #220 into master will increase coverage by 0.25%

@@             master       #220   diff @@
==========================================
  Files            91         89     -2   
  Lines          4697       4516   -181   
  Methods           0          0          
  Messages          0          0          
  Branches        811        764    -47   
==========================================
- Hits           3360       3242   -118   
+ Misses          861        836    -25   
+ Partials        476        438    -38

Powered by Codecov. Last update 69ac11e...eb4ab11

jerryshao · 2016-11-03T08:01:12Z

server/src/main/scala/com/cloudera/livy/server/recovery/SessionStore.scala

      .map(_.nextSessionId).getOrElse(0)
+    store.set(sessionManagerPath(sessionType), SessionManagerState(nextSessionId + 1))
+    nextSessionId


IIUC, the nextSessionId you get is the previous one persisted and you didn't increase the number when returned.

Also each time when you get the session id, you will fetch it from persisted storage, I think it is not necessary and will bring in extra overhead.

@jerryshao The scala doc comments are a bit misleading. The value stored in session store is the next unused session ID.
Performance wise, this change calls the session store as many times as the previous code. The only difference is that previously SessionManager was calling SessionStore.set, but now the SessionStore calls it.
Please take a look at SessionManager (lines 81 to 84) in 5e8474e (

livy/server/src/main/scala/com/cloudera/livy/sessions/SessionManager.scala

Line 81 in 5e8474e

def nextId(): Int = synchronized {

)

Sorry about the poor doc. My bad. Would you mind fixing it?
I think fixing the doc and rename val nextSessionId = store.get to lastUsedSessionId?

I have to correct myself. Prior to this commit, there was one read from SessionStore on recovery or on server start. There was also one write per nextId() call. Now we have one read and one write to SessionStore per nextId() call.

I think this's necessary for HA consistency.
Can we come up with some smart way to not pay this price for recovery mode, but just for HA mode?

This is my initial thoughts on it.
We can refactor the logic to generate sessionIDs into a separate trait/class and have two implementation: one for single-node, and one for multi-node. The single node mode can simply use a local atomic value, but the multi-node mode should use a more sophisticated implementation to guarantee uniqueness and consistency across all nodes.

I think we are thinking similarly but have 1 difference. You are adding the session id generation to SessionStore while I'm thinking about adding it to StateStore.

I would prefer to keep HA logic out of SessionStore and move these logic into ZookeeperStateStore. Having a single code path in SessionStore makes isolating HA related changes easier.

alex-the-man · 2016-11-03T19:10:23Z

Is this done for implementing HA?

meisam · 2016-11-03T19:15:39Z

@tc0312 Yes. This change is needed to implement multimode HA.

alex-the-man · 2016-11-03T19:39:41Z

server/src/main/scala/com/cloudera/livy/server/recovery/SessionStore.scala

   * If no SessionManagerState is stored, it returns 0.
+    * It saves the new session ID to the session store.


Nit: Extra space here.

alex-the-man · 2016-11-03T19:40:17Z

server/src/main/scala/com/cloudera/livy/server/recovery/SessionStore.scala

      .map(_.nextSessionId).getOrElse(0)
+    store.set(sessionManagerPath(sessionType), SessionManagerState(nextSessionId + 1))
+    nextSessionId


Sorry about the poor doc. My bad. Would you mind fixing it?
I think fixing the doc and rename val nextSessionId = store.get to lastUsedSessionId?

alex-the-man · 2016-11-03T19:44:00Z

server/src/main/scala/com/cloudera/livy/server/recovery/SessionStore.scala

-  def getNextSessionId(sessionType: String): Int = {
-    store.get[SessionManagerState](sessionManagerPath(sessionType))
+  def getNextSessionId(sessionType: String): Int = synchronized {
+    val nextSessionId = store.get[SessionManagerState](sessionManagerPath(sessionType))


Reviewing this with HA in mind. I think it's possible that another livy-server instance updates SessionManagerState between the store.get() and store.set() call.
I think we should add locking/optimistic locking + retry here to avoid servers stepping on each other. Or better, we change the interface of StateStore to make use of Sequence Nodes in ZooKeeper.

That's right! But for multi-node HA, I am overriding this method in ZooKeeperStore and I am using a distributed atomic long recipe (https://curator.apache.org/curator-recipes/distributed-atomic-long.html) to guarantee that generated session IDs are unique and are generated and stored atomically. But for single node HA synchronizing on the session store instance would suffice.

With your ZooKeeperStore change, who's doing nextSessionId + 1?

SessionStore calls into the distributed Atomic recipe from Apache curator.
This is the how I am doing it:

val distributedSessionId = new DistributedAtomicLong(curatorClient, zkPath, retryPolicy) def nextBatchSessionId: Int = { recursiveTry(distributedSessionId, MAX_RETRY) match { case Some(sessionId) => sessionId.toInt case None => val msg: String = "Failed to get the next session id from Zookeeper" logger.warn(msg) throw new IOException(msg) } } @tailrec private def recursiveTry(distributedLong: DistributedLong, retryCount: Int): Option[Long] = { val updatedValue = distributedLong.increment updatedValue.succeeded match { case _ if retryCount <= 0 => None case true if retryCount > 0 => Option(updatedValue.preValue()) case _ => recursiveTry(distributedLong, retryCount - 1) } }

Are these codes in SessionStore or a subclass of SessionStore?

Do you mean ZooKeeperStateStore?

Yes. it is in ZooKeeperStateStore. Sorry!

Did you add a new method in StateStore to generate unique id?

Yes I did. But as you said in your comment, moving the logic out of SessionStore makes a lot more sense.

I would prefer to keep HA logic out of SessionStore and move these logic into ZookeeperStateStore. Having a single code path in SessionStore makes isolating HA related changes easier.

I am going to update the pull request based on the feedback.

alex-the-man · 2016-11-03T19:45:05Z

server/src/main/scala/com/cloudera/livy/sessions/SessionManager.scala

-    sessionStore.saveNextSessionId(sessionType, idCounter.get())
-    id
-  }
+  // sessionStore.getNextSessionId is guaranteed to return  atomic and returns unique IDs.


Nit: Extra space between return & atomic.

jerryshao · 2016-11-04T05:29:37Z

Can we have a whole plan of Livy HA before doing this small refactoring work? My concern is that currently we don't have a decided plan of Livy HA, if finally the plan is changed or updated, current changes may be obsolete.

meisam · 2016-11-04T18:28:43Z

@jerryshao There is a JIRA ticket open for it: https://issues.cloudera.org/browse/LIVY-231
Desing doc: https://github.com/meisam/livy/wiki/Multi-node-HA-for-batch-sessions
preliminary pull request for LIVY-231: #222

to SessionStore Task-url: https://issues.cloudera.org/browse/LIVY-239

This commit also - updates the test cases around SessionStore - Removes the SessionManagerState class (Not neede anymore). We are storing a Long value that can be incremented. Task-url: https://issues.cloudera.org/browse/LIVY-239

jerryshao reviewed Nov 3, 2016

View reviewed changes

alex-the-man reviewed Nov 3, 2016

View reviewed changes

meisam mentioned this pull request Nov 4, 2016

LIVY-231: Multi node HA for batch sessions #222

Open

alex-the-man added this to the HA milestone Nov 8, 2016

meisam added 2 commits November 9, 2016 10:08

LIVY-239: Moving the logic to generate session IDs from Session Manager

d2fa391

to SessionStore Task-url: https://issues.cloudera.org/browse/LIVY-239

meisam force-pushed the LIVY-239 branch from c5fb9ea to eb4ab11 Compare November 9, 2016 18:13

alex-the-man modified the milestones: HA, 0.4 Dec 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LIVY-239: Moving the logic to generate session IDs from Session Manager to SessionStore #220

LIVY-239: Moving the logic to generate session IDs from Session Manager to SessionStore #220

meisam commented Nov 3, 2016

codecov-io commented Nov 3, 2016 •

edited

Loading

jerryshao Nov 3, 2016

meisam Nov 3, 2016 •

edited

Loading

alex-the-man Nov 3, 2016

meisam Nov 3, 2016

alex-the-man Nov 3, 2016

meisam Nov 3, 2016

alex-the-man Nov 3, 2016

alex-the-man commented Nov 3, 2016

meisam commented Nov 3, 2016

alex-the-man Nov 3, 2016

alex-the-man Nov 3, 2016

alex-the-man Nov 3, 2016 •

edited

Loading

meisam Nov 3, 2016

alex-the-man Nov 3, 2016

meisam Nov 3, 2016

alex-the-man Nov 3, 2016

alex-the-man Nov 3, 2016

meisam Nov 3, 2016

alex-the-man Nov 3, 2016

meisam Nov 3, 2016

meisam Nov 3, 2016

alex-the-man Nov 3, 2016

jerryshao commented Nov 4, 2016

meisam commented Nov 4, 2016

		* If no SessionManagerState is stored, it returns 0.
		* It saves the new session ID to the session store.

LIVY-239: Moving the logic to generate session IDs from Session Manager to SessionStore #220

Are you sure you want to change the base?

LIVY-239: Moving the logic to generate session IDs from Session Manager to SessionStore #220

Conversation

meisam commented Nov 3, 2016

codecov-io commented Nov 3, 2016 • edited Loading

Current coverage is 71.78% (diff: 54.54%)

Choose a reason for hiding this comment

meisam Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-the-man commented Nov 3, 2016

meisam commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-the-man Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Nov 4, 2016

meisam commented Nov 4, 2016

codecov-io commented Nov 3, 2016 •

edited

Loading

meisam Nov 3, 2016 •

edited

Loading

alex-the-man Nov 3, 2016 •

edited

Loading