[LIVY-41] Let users access sessions by session name by meisam · Pull Request #48 · apache/livy

meisam · 2017-09-19T21:17:24Z

This commit enables Livy users to access sessions either by names or by auto-generated sessiond id's.

It also prevents users from creating sessions that have the same name.

This commit keeps API change minimal. These are places that API change
is needed:

Session and its sub-classes adds a new field, name.
RecoveryMetadata and its sub-classes adds a new field, name.
SessionManager adds a new method getSession(name: String) which looks sessions up by name.

Task-url: https://issues.apache.org/jira/browse/LIVY-41

This commit enables Livy users to access sessions either by names or by auto-generated sessiond id's. It also prevents users from creating sessions that have the same name. This commit keeps API change minimal. Thse are palces that API change is needed: - `Session` and its subclasses adds a new field, `name`. - `RecoveryMetadata` and its subclasses adds a new field, `name`. - `SessionManager` adds a new method `getSession(name: String)` which lookups sessions by name. A more clean implementation would change the signature of `SessionManager.register` so it returns a proper container around the session value to determine if it failed to register the given session. For example, ```Scala def register(session S): Either[S, Throwable] ``` Task-url: https://issues.apache.org/jira/browse/LIVY-41

codecov-io · 2017-09-19T23:41:52Z

Codecov Report

Merging #48 into master will increase coverage by 0.36%.
The diff coverage is 80%.

@@             Coverage Diff              @@
##             master      #48      +/-   ##
============================================
+ Coverage     68.34%   68.71%   +0.36%     
- Complexity      895      902       +7     
============================================
  Files           100      100              
  Lines          5604     5644      +40     
  Branches        840      847       +7     
============================================
+ Hits           3830     3878      +48     
+ Misses         1225     1211      -14     
- Partials        549      555       +6

Impacted Files	Coverage Δ	Complexity Δ
.../scala/org/apache/livy/server/SessionServlet.scala	`68.96% <100%> (+1.89%)`	`17 <4> (+1)`	⬆️
...apache/livy/server/batch/BatchSessionServlet.scala	`90.32% <100%> (+1.86%)`	`3 <0> (ø)`	⬇️
...la/org/apache/livy/server/batch/BatchSession.scala	`86.17% <100%> (+0.77%)`	`14 <5> (+1)`	⬆️
...va/org/apache/livy/client/common/HttpMessages.java	`95.23% <100%> (+0.11%)`	`0 <0> (ø)`	⬇️
...server/interactive/InteractiveSessionServlet.scala	`66.93% <100%> (+0.54%)`	`6 <0> (ø)`	⬇️
.../main/scala/org/apache/livy/sessions/Session.scala	`72.56% <50%> (-1.02%)`	`20 <5> (+4)`
...e/livy/server/interactive/InteractiveSession.scala	`69.04% <71.05%> (+0.68%)`	`44 <4> (+1)`	⬆️
...cala/org/apache/livy/sessions/SessionManager.scala	`82.02% <75%> (-0.7%)`	`24 <1> (+1)`
...ain/scala/org/apache/livy/utils/SparkYarnApp.scala	`72.53% <0%> (-0.71%)`	`33% <0%> (ø)`
...ain/java/org/apache/livy/rsc/driver/RSCDriver.java	`78.81% <0%> (-0.43%)`	`41% <0%> (-1%)`
... and 5 more

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 02550f7...9c64c81. Read the comment docs.

ajbozarth

I made a handful of comments, still not 100% behind this, will need some time to think about it.

A couple of general comments:

You'll need to update the Docs to match the API updates.

I think you should use a uniform naming convention for the test classes, it seems each class has it's own naming convention.

ajbozarth · 2017-09-20T00:23:13Z

      checkFn: Option[(String, HttpServletRequest) => Boolean]): Any = {
-    val sessionId = params("id").toInt
-    sessionManager.get(sessionId) match {
+    val idParam: String = params("id")


I don't think re-using the id param for name is a good idea. It should have it's own optional param instead. I believe this is already how you are doing session creation anyway.

I don't have any strong opinion for or against reusing id param. Here is why:

If we do not reuse id param, we have to create a new REST endpoint for each existing one. Reusing id param allows Livy users access a session seamlessly by its ID or its name through the same endpoint:

sessions/{id} or sessions/{name}
sessions/{id}/statements or sessions/{name}/statements
sessions/{id}/state or sessions/{name}/state
sessions/{id}/log or sessions/{name}/log

If we do not reuse id param, we have to add separate endpoints for accessing sessions by name. Something like
named-sessions/{name}
named-sessions/{name}/statements
named-sessions/{name}/state
named-sessions/{name}/log
or
sessions/by-name/{name}
sessions/by-name/{name}/statements
sessions/by-name/{name}/state
sessions/by-name/{name}/log

or
sessions?name={name}
sessions/statements?name={name}
sessions/state?name={name}
sessions/log?name={name}

@ajbozarth, what are your thoughts on adding a new endpoint?

I checked the code to followup my argument and realized that the way these end-points are written the way you did it is actually the best way. The param name id is not user-facing so the clarity I want can be made in the Docs rather than the code. Given this change to the API though I would like you to double check that the Web UI will support these changes, both that it still works with the id, but also works by using a session name in the url. Actually adding the Session name to the UI can be done in a followup task (which I'm willing to do).

Let's rename this variable as idOrNameParam to be clear.

👍 I'll rename it.

ajbozarth · 2017-09-20T00:25:18Z

+    val sessionName: String = createRequest.name match {
+      case Some(name) if sessionManager.get(name).isEmpty =>
+        name
+      case Some(name) =>


I think this should be an else statement in the first case instead. This usage of match is a bit hacky.

👍 I'll update this even though I am an advocate of if-guards in case statements.

I was thinking an if/else inside the Some(name) case.

ajbozarth · 2017-09-20T00:26:53Z

+          s"Choose a different name or delete the existing session."
+        throw new IllegalArgumentException(msg)
+      case None =>
+        s"INTERACTIVE-SESSION-$sessionId"


This wasn't changed in the copy-paste. Also I'm not a fan of this default name, it's a bit verbose. Just batch-$sessionId would suffice.

👍 I'll update it.

ajbozarth · 2017-09-20T00:28:21Z

+          s"Choose a different name or delete the existing session."
+        throw new IllegalArgumentException(msg)
+      case None =>
+        s"INTERACTIVE-SESSION-$sessionId"


As above, just session-$sessionId would suffice.

ajbozarth · 2017-09-20T00:28:28Z

+    val sessionName: String = createRequest.name match {
+      case Some(name) if sessionManager.get(name).isEmpty =>
+        name
+      case Some(name) =>


same as above

ajbozarth · 2017-09-20T00:31:14Z

+      // Livy checks to make sure another session with the same name does not exist already.
+      // This case should not happen rarely.
+      // already exists. But looking up a session name and adding it to the set of existing sessions
+      // should happen atomically.


Theres some confusing wording this comment block, might be artifacts from changes during development, but it makes the comment difficult to understand.

ajbozarth · 2017-09-20T00:32:56Z

+      if (sessionsByName.contains(session.name)) {
+        val msg = s"Session ${session.name} already exists!"
+        error(msg)
+        delete(session)


Discussion Point: Do we want to fail a request with a "bad" name or should we instead give a warning message and create a session with a default name instead? Just an idea, I'm still not 100% sure of this PR's use case.

This is an example use case we have here. I'll ask Prabhu to provide more.

Team AAA has an ETL pipeline everyday at 1pm. The pipeline is a chain of jobs that work on the same dataset. They want to create a session on Livy and use it to run the pipeline. They also want to push the metrics and stats of their pipeline to alerting and monitoring services.
This PR allows them to access the session, manage it, and monitor it with a human readable name like team_aaa_daily_etl.

Your example use case does help me with understanding this PR's purpose, which I am liking more, but not this particular code decision. Is there a good reason we should be failing session creation on a "bad" session name rather than displaying a warning that the name was bad then creating a session with the default naming? I'm not saying either way is better, I am just wondering you reason for doing it this way.

I think fail the request and return user the specific bad http code would be better then choosing another name for user. Because user want to create a session with name "A" for example, if Livy cannot satisfy user's requirement, then user should choose another name. It is better to let user to decide than letting Livy to handle it.

jerryshao · 2017-09-20T03:29:29Z

@meisam , do you need to change Java and python job API to support this feature? As I remembered it can specify session id to reuse the existing session, so for now we should also support session name.

meisam · 2017-09-20T23:41:09Z

@jerryshao Yes, changing the Java and python API is a good idea.

ajbozarth · 2017-09-21T21:14:41Z

IMPO, for quicker pr reviews, I think adding Java and python API support can be done in a followup PR

jerryshao · 2017-09-28T05:57:19Z

  }

  def startBatch(
+      name: String,


I would suggest to use Option[String] instead to avoid explicitly set to "null" as below.

I'll update it to avoid explicit null values.

jerryshao · 2017-09-28T05:59:43Z

      checkFn: Option[(String, HttpServletRequest) => Boolean]): Any = {
-    val sessionId = params("id").toInt
-    sessionManager.get(sessionId) match {
+    val idParam: String = params("id")


Let's rename this variable as idOrNameParam to be clear.

jerryshao · 2017-09-28T06:07:48Z

+        // it prevents Livy from submitting applications to Spark.
+        val msg = s"Session $name already exists! " +
+          s"Choose a different name or delete the existing session."
+        throw new IllegalArgumentException(msg)


Is the exception here OK, shall we return some HTTP code to rest client about session name duplication?

nice catch, I not sure this would be surfaced to the submitter or not

One case that we should keep in mind about returning an error code.
Assume the existing session s with name daily-job.
If the owner s is requesting a new session with name daily-job, we will reach this point and the user gets a http 400 bad request.
But if a different user is requesting a session s2 with name daily-job, the user gets a 403 Forbidden before reaching this point, because he/she does not have access to /sessions/daily-job/.

Yeah, that's true.

jerryshao · 2017-09-28T06:10:19Z

    val proxyUser = checkImpersonation(createRequest.proxyUser, req)
+    val sessionId: Int = sessionManager.nextId()
+    val sessionName: String = createRequest.name match {
+      case Some(name) if sessionManager.get(name).isEmpty =>


We'd better put this code into common place for both interactive and batch session.

It is refactored into one place now.

jerryshao · 2017-09-28T06:15:26Z


  override def recoveryMetadata: RecoveryMetadata =
-    BatchRecoveryMetadata(id, appId, appTag, owner, proxyUser)
+    BatchRecoveryMetadata(id, name, appId, appTag, owner, proxyUser)


@meisam what if old recovery metadata which doesn't have name field, I think we should consider the compatibility of old metadata file with new Livy server, would you please check it?

What do you suggest? Should we assign such sessions auto-generated names on recovery? Or should we leave them without a name?

I'm saying with your changes, if new Livy server tries to recover from old data, what will be happened? Will it be failed?

Since the old recovery data doesn't have a session name field, so we could offer an auto-generated name, but we should not fail such scenario.

I'll add test cases to confirm that Livy can recover from old data.

jerryshao · 2017-09-28T06:17:01Z


  protected[this] final val idCounter = new AtomicInteger(0)
  protected[this] final val sessions = mutable.LinkedHashMap[Int, S]()
+  private[this] final val sessionsByName = mutable.Map[String, S]()


I think we can use mutable.HashMap to be clear.

Addressed 👍

jerryshao · 2017-09-28T06:22:48Z

+      if (sessionsByName.contains(session.name)) {
+        val msg = s"Session ${session.name} already exists!"
+        error(msg)
+        delete(session)


I think fail the request and return user the specific bad http code would be better then choosing another name for user. Because user want to create a session with name "A" for example, if Livy cannot satisfy user's requirement, then user should choose another name. It is better to let user to decide than letting Livy to handle it.

meisam · 2017-09-29T19:53:02Z

I think fail the request and return user the specific bad http code would be better then choosing another name for user. Because user want to create a session with name "A" for example, if Livy cannot satisfy user's requirement, then user should choose another name. It is better to let user to decide than letting Livy to handle it.

I like failing the request option more. The only caveat is that with current approach, sometimes we submit the spark App and then kill it later (if two requests arrive at almost the same time, we launch both, but later when we want to add them to sessionManager, we have to keep only one and kill and and discard the other one).

One way to avoid that is to refactor Session, InteractiveSesion, and BatchSession so it does not start the Spark process immediately, so we can start the Spark process after registering them in SessionManager.

Are you okay with this approach?

ajbozarth · 2017-09-29T22:52:22Z

Are you okay with this approach?

I would have to see the change first but I think this could potentially be ok

jerryshao · 2017-09-30T01:17:44Z

I like failing the request option more. The only caveat is that with current approach, sometimes we submit the spark App and then kill it later (if two requests arrive at almost the same time, we launch both, but later when we want to add them to sessionManager, we have to keep only one and kill and and discard the other one).

That's the problem of concurrency, I think we can first check the validity of session name, if it is conflicted, then directly return bad code. We should not postpone such check until session is created.

jerryshao · 2017-09-30T01:23:10Z

One way to avoid that is to refactor Session, InteractiveSesion, and BatchSession so it does not start the Spark process immediately, so we can start the Spark process after registering them in SessionManager.

@meisam I would suggest to leave this work to another PR for the simplicity of review.

Signed-off-by: Fathi Salmi, Meisam(mfathisalmi) <mfathisalmi@paypal.com>

Changing ``` case ... if .... => ``` to ``` case ... => if ... ```

jerryshao · 2017-10-13T03:02:42Z

@meisam can you please address the comments, thanks!

If no session name is provided, do not use session names. This makes it possible to recover sessions that were stored with no name.

meisam · 2017-10-17T00:03:01Z

This is the output from last build:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated.

Closing and re-opening this pull request to trigger a new build.

meisam · 2017-10-17T00:48:19Z

@meisam can you please address the comments, thanks!

@jerryshao I addressed comments in the previous commit.

jerryshao · 2017-10-19T02:39:39Z

    (scriptPath: String, args: List[String], sparkConf: Map[String, String] = Map.empty)
    (f: (LivyRestClient#BatchSession) => R): R = {
-    val s = livyClient.startBatch(scriptPath, None, args, sparkConf)
+    val s = livyClient.startBatch(null, scriptPath, None, args, sparkConf)


Should here change to None?

That's right. I prefer passing None to passing null. I'll fix it.

jerryshao · 2017-10-19T02:39:51Z

    (testClass: Class[_], args: List[String], sparkConf: Map[String, String] = Map.empty)
    (f: (LivyRestClient#BatchSession) => R): R = {
-    val s = livyClient.startBatch(testLibPath, Some(testClass.getName()), args, sparkConf)
+    val s = livyClient.startBatch(null, testLibPath, Some(testClass.getName()), args, sparkConf)


I'll fix this one too.

jerryshao · 2017-10-19T02:40:02Z

      heartbeatTimeoutInSecond: Int = 0)
    (f: (LivyRestClient#InteractiveSession) => R): R = {
-    withSession(livyClient.startSession(kind, sparkConf, heartbeatTimeoutInSecond)) { s =>
+    withSession(livyClient.startSession(null, kind, sparkConf, heartbeatTimeoutInSecond)) { s =>


jerryshao · 2017-10-19T02:46:18Z

  sessionStore.save(RECOVERY_SESSION_TYPE, recoveryMetadata)
  heartbeat()

-  private val app = mockApp.orElse {


What's the purpose of below change?

Before this change, Livy immediately submits the Spark app upon receiving the request. After this change, Livy has to call Session.start() to submit the Spark app, which means Livy can make sure another session with the same name does not exist before submitting the Spark app.

Without this change, making session names unique is complicated, because if two session names collide, we have only two choices:

Kill one of the sessions.

Come up with a logic that gives one of the sessions an autogenerated name and guarantees the autogenerated name does not collide with any of the existing/future session names.

Thanks for the explanation, let me review again.

meisam · 2018-12-13T20:59:41Z

Is there a plan to merge this PR? If so, I can fix the recent merge conflicts. Otherwise we should close it for good.

vanzin

There seems to be previous feedback that hasn't been addressed.

I took a quick look but it seems to me this is putting both IDs and names into the same namespace. Except I didn't notice any enforcement that IDs and names do not conflict. So if I create a session with name "1", and a later request makes Livy try to create a session with ID 1, what happens?

I think it would be better to keep both concepts separate (e.g. how on /dev/disk you have multiple non-conflicting ways of addressing the same disk).

vanzin · 2018-12-19T00:40:29Z

+      val driverProcess = client.flatMap { c => Option(c.getDriverProcess) }
+        .map(new LineBufferedProcess(_, livyConf.getInt(LivyConf.SPARK_LOGS_SIZE)))
+      driverProcess.map { _ => SparkApp.create(appTag, appId, driverProcess, livyConf, Some(this))
+      }


either in previous line or, if it doesn't fit, use multi-line syntax for the whole closure.

In these lines, I only changed the indentation. I think formatting issues should be handled in a separate PR if needed.

vanzin · 2018-12-19T00:40:43Z

+    } else {
+      val uriFuture = Future { client.get.getServerUri.get() }
+
+      uriFuture onSuccess { case url =>


uriFuture.onSuccess, also in other places

Addressed 👍

vanzin · 2018-12-19T00:42:05Z

  def register(session: S): S = {
    info(s"Registering new session ${session.id}")
    synchronized {
+      session.name.map { sessionName =>


s/map/foreach

Addressed 👍

vanzin · 2018-12-19T00:44:11Z

-    val sessionId = params("id").toInt
-    sessionManager.get(sessionId) match {
+    val idOrNameParam: String = params("id")
+    val session = if (idOrNameParam.forall(_.isDigit)) {


You're assuming that a string with all digits is an ID and not a name. I don't see that enforced when creating a session?

I changed this to force session names to have at least one non-digit character.

Session names cannot be numbers.

# Conflicts: # server/src/main/scala/org/apache/livy/server/SessionServlet.scala # server/src/main/scala/org/apache/livy/server/batch/BatchSessionServlet.scala # server/src/main/scala/org/apache/livy/server/interactive/InteractiveSession.scala # server/src/test/scala/org/apache/livy/server/batch/BatchServletSpec.scala # server/src/test/scala/org/apache/livy/server/batch/BatchSessionSpec.scala # server/src/test/scala/org/apache/livy/server/interactive/InteractiveSessionServletSpec.scala # server/src/test/scala/org/apache/livy/server/interactive/InteractiveSessionSpec.scala

A session name should have at least one non digit character in it.

vanzin

Mostly minor things.

vanzin · 2019-01-29T19:08:31Z

+    str.nonEmpty && str.forall(_.isDigit)
+  }
+
+  name.foreach { idStr =>


idStr is a misleading name since this is not an id.

renamed it to sessionName

vanzin · 2019-01-29T19:09:14Z

  protected implicit val executionContext = ExecutionContext.global

+  private def isSessionId(str: String): Boolean = {
+    str.nonEmpty && str.forall(_.isDigit)


An empty name should be invalid too, right?

I think it would be clearer to have this code inlined below, as I suggested before. Then that whole block becomes the clear place where session name validation occurs.

An empty name should be invalid too, right?
Right.

While testing what is a good way to separate session names from session IDs, I saw two interesting behaviors in Livy (and Scala/Java).

Char.isDigit returns true for digits in all alphabets systems. For example:

scala> "᧑᧒᧓᧔᧕".forall(_.isDigit) res18: Boolean = true scala> "᧑᧒᧓᧔᧕".toInt res19: Int = 12345 scala> "᧑᧒᧓᧔᧕".map(c => Character.getName(c.toInt)) res20: scala.collection.immutable.IndexedSeq[String] = Vector(NEW TAI LUE DIGIT ONE, NEW TAI LUE DIGIT TWO, NEW TAI LUE DIGIT THREE, NEW TAI LUE DIGIT FOUR, NEW TAI LUE DIGIT FIVE)

If I create a session with sessionID=1 all these are valid endpoints to the session:

${LIVY_SERVER}:${LIVY_PORT}/sessions/1 ${LIVY_SERVER}:${LIVY_PORT}/sessions/01 ${LIVY_SERVER}:${LIVY_PORT}/sessions/001 ${LIVY_SERVER}:${LIVY_PORT}/sessions/00001 ${LIVY_SERVER}:${LIVY_PORT}/sessions/۱ ${LIVY_SERVER}:${LIVY_PORT}/sessions/0۱ ${LIVY_SERVER}:${LIVY_PORT}/sessions/۰۰۰۱ ${LIVY_SERVER}:${LIVY_PORT}/sessions/0۰۰00۰᧑

Should we keep this behavior as is?

For now, disallowing empty strings as session names.

vanzin · 2019-01-29T19:16:11Z

+      view.log shouldEqual log
+    }
+
+    it("should show session properties for sessions without a name") {


This looks very much like the test above. You could have:

Seq( (Some(id), None), (None, Some(name) ).foreach { case (id, name) => it("should show session properties (id = $id, name = $name)") { methodWithTestImpl(id, name) } }

refactored into one helper method.

vanzin · 2019-01-29T19:17:33Z

+      val batch = BatchSession.recover(m, conf, sessionStore, Some(mockApp))
+
+      batch.state shouldBe (SessionState.Recovering)
+


Check that the recovered session is named?

Added the check.

vanzin · 2019-01-29T19:18:31Z

+    view.log shouldEqual log.asJava
+  }
+
+  it("should show session properties for sessions without a name") {


Same thing here. Better to avoid copy & pasting the existing test and changing a couple of lines.

refactored into one helper method.

vanzin · 2019-01-29T19:19:05Z

+        MockitoMatchers.eq(InteractiveSession.RECOVERY_SESSION_TYPE), anyObject())
+    }
+
+    it("should recover sessions with no name") {


Another one that looks like copy & paste of the existing test.

refactored into one helper method.

vanzin · 2019-01-29T19:19:47Z

+          |  "version": 1
+          |}
+        """.stripMargin
+      val batchRecoveryMetadata = stateStore.deserialize[BatchRecoveryMetadata](json.getBytes())


nit: getBytes(UTF_8)

vanzin · 2019-01-29T19:21:35Z

+      an[IllegalArgumentException] should be thrownBy manager.register(session2)
+      manager.get(session1.id).isDefined should be(true)
+      manager.get(session2.id).isDefined should be(false)
+      eventually(timeout(5 seconds), interval(100 millis)) {


This test is not needed here, right? It has nothing to do with the session name.

removed the garbage collection section.

vanzin · 2019-01-29T19:22:00Z

+      val livyConf = new LivyConf()
+      val name = "Mock-session"
+      livyConf.set(LivyConf.SESSION_TIMEOUT, "100ms")
+      val manager = new SessionManager[MockSession, RecoveryMetadata](


This is now in 3 different places in this file. Should probably be a helper method.

Refactored!

alex-the-man · 2019-01-29T22:35:57Z

Have we considered the potential impact of this change for Livy multi-master HA?
To support multi-master HA without this feature, the only thing we need is a distributed atomic counter (which ZK offers).
With this change, we have to enforce session names are unique across multiple masters. This makes multi-master HA more complicated.

meisam · 2019-01-30T00:07:37Z

To support multi-master HA without this feature, the only thing we need is a distributed atomic counter (which ZK offers).
With this change, we have to enforce session names are unique across multiple masters. This makes multi-master HA more complicated.

Very good point.
One way to enforce unique session names is to write the name of the session to Zookeeper. If a second session manager tries creating a ZK node with the same node, it will fail.

Also make it reject empty names

vanzin · 2019-02-05T00:35:58Z

Just to refresh my memory: is multi-master HA a thing? I looked at how session IDs are created today and it doesn't really seem that atomic when you have multiple servers looking at the store. In fact it doesn't seem that atomic even with a single server:

    val id = idCounter.getAndIncrement()
    sessionStore.saveNextSessionId(sessionType, idCounter.get())

That can write the wrong id to the session store if two threads race in just the wrong way.

Anyway, if multi-master is not a thing today, then this looks ok to me.

meisam · 2019-02-05T00:57:45Z

I looked at how session IDs are created today and it doesn't really seem that atomic ...

There is a separate PR to make sessionIDs atomic across multiple servers.
#103

vanzin · 2019-02-05T01:02:50Z

So I guess pushing this change will require changes in that PR, or vice-versa. Seems this one is farther along.

meisam · 2019-02-05T01:06:35Z

So I guess pushing this change will require changes in that PR, or vice-versa. Seems this one is farther along.

No. #103 is not needed for this PR. #103 is part of a bigger PR to implement multi-server HA.

vanzin · 2019-02-05T01:09:50Z

I didn't mean it's needed. I'm saying that the "how to have unique names with multiple servers" issue becomes a problem when either PR goes in.

So when I push this, that problem will have to be solved in the other PR. Or vice-versa, if the other PR went in first.

vanzin · 2019-02-05T19:24:39Z

Merging to master.

This commit enables Livy users to access sessions either by names or by auto-generated sessiond id's. It also prevents users from creating sessions that have the same name. This commit keeps API change minimal. These are places that API change is needed: - `Session` and its sub-classes adds a new field, `name`. - `RecoveryMetadata` and its sub-classes adds a new field, `name`. - `SessionManager` adds a new method `getSession(name: String)` which looks sessions up by name. Task-url: https://issues.apache.org/jira/browse/LIVY-41 Author: Fathi Salmi, Meisam(mfathisalmi) <mfathisalmi@paypal.com> Author: Meisam Fathi <meisam.fathi@gmail.com> Author: Fathi Salmi, Meisam <meisam.fathi@gmail.com> Author: Fathi, Meisam <meisam.fathi@gmail.com> Closes apache#48 from meisam/LIVY-41-rebased.

ajbozarth reviewed Sep 20, 2017

View reviewed changes

jerryshao reviewed Sep 28, 2017

View reviewed changes

meisam added 3 commits October 5, 2017 16:15

[LIVY-41] renaming id tp idOrName

3f28b61

Signed-off-by: Fathi Salmi, Meisam(mfathisalmi) <mfathisalmi@paypal.com>

[LIVY-41] refactoring guareded case statements

dc644c7

Changing ``` case ... if .... => ``` to ``` case ... => if ... ```

First shot at making brnach names optional

967e896

[LIVY-41] Making session names optional

0a0e1cf

If no session name is provided, do not use session names. This makes it possible to recover sessions that were stored with no name.

meisam closed this Oct 17, 2017

meisam reopened this Oct 17, 2017

jerryshao reviewed Oct 19, 2017

View reviewed changes

vanzin reviewed Dec 19, 2018

View reviewed changes

meisam added 4 commits January 14, 2019 15:56

Change name from null to None in test cases

c16685a

Validate session names

c34eca8

Session names cannot be numbers.

Exclude numbers from valid session names.

c995ab2

A session name should have at least one non digit character in it.

Start heartbeat and save session recovery data after start() is called.

9c64c81

vanzin reviewed Jan 29, 2019

View reviewed changes

meisam added 5 commits January 29, 2019 16:22

Inline check for session names.

516a880

Also make it reject empty names

Refactor common test case code snippets into helper functions

334b165

Decode string bytes as "UTF-8"

ef56dea

Remove the irrelevant checks for the duplicate session names test

41ff9aa

Guard if caluse with braces

d673baf

meisam closed this Jan 30, 2019

meisam reopened this Jan 30, 2019

meisam added 3 commits January 30, 2019 07:16

[LIVY-41] Fix typo : "UTF_8" -> "UTF-8"

a43a968

[LIVY-41] Fix tests for session names

35c34d7

[LIVY-41] Remove ID parameter from testShowSessionProperties

518bfdf

meisam closed this Jan 30, 2019

meisam reopened this Jan 30, 2019

Fix the test cases after refactoring

f61c3d5

meisam closed this Jan 31, 2019

meisam reopened this Jan 31, 2019

asfgit closed this in def6318 Feb 5, 2019

meisam deleted the LIVY-41-rebased branch February 5, 2019 22:30

patricker mentioned this pull request Jun 16, 2020

NIFI-6175 Spark Livy - Improving Livy apache/nifi#3511

Closed

11 tasks

		val batch = BatchSession.recover(m, conf, sessionStore, Some(mockApp))

		batch.state shouldBe (SessionState.Recovering)

Uh oh!

Conversation

meisam commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ajbozarth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meisam Sep 20, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryshao commented Sep 20, 2017

Uh oh!

meisam commented Sep 20, 2017

Uh oh!

ajbozarth commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meisam commented Sep 19, 2017 •

edited

Loading

codecov-io commented Sep 19, 2017 •

edited

Loading

meisam Sep 20, 2017 •

edited

Loading

jerryshao commented Sep 30, 2017 •

edited

Loading