[LIVY-758] Document how to attach to an existing session from Java client by tmnd1991 · Pull Request #290 · apache/livy

tmnd1991 · 2020-04-16T19:20:37Z

What changes were proposed in this pull request?

Add an explicit way to set the session when connecting from the Java client.
https://issues.apache.org/jira/browse/LIVY-758

How was this patch tested?

Unit tested through HttpClientSpec.

tmnd1991 · 2020-04-16T19:23:20Z

I could not assign myself the relate jira, how can I do it? @mgaido91 @vanzin @jerryshao

mgaido91 · 2020-04-16T19:57:15Z

@tmnd1991 you can't, JIRAs are assigned when related patches are merged and they are resolved

mgaido91 · 2020-04-16T19:57:54Z


  private static final ServiceLoader<LivyClientFactory> CLIENT_FACTORY_LOADER =
-    ServiceLoader.load(LivyClientFactory.class, classLoader());
+      ServiceLoader.load(LivyClientFactory.class, classLoader());


nit: unneeded change, please revert

mgaido91 · 2020-04-16T20:03:04Z

+   * @param uri the uri of the livy server,
+   *            if the uri contains <pre>sessions/{sessionId}</pre>
+   *            the client will connect to the existing session,
+   *            otherwise it will create a new session.


Suggested change

* @param uri the uri of the livy server,

* if the uri contains <pre>sessions/{sessionId}</pre>

* the client will connect to the existing session,

* otherwise it will create a new session.

* @param uri The URI of Livy server. If the URI contains <pre>sessions/{sessionId}</pre>,

* the client will connect to the specified existing session, otherwise it will create a

* new session.

mgaido91 · 2020-04-16T20:03:40Z

+   *            if the uri contains <pre>sessions/{sessionId}</pre>
+   *            the client will connect to the existing session,
+   *            otherwise it will create a new session.
+   * @return the builder itself.


Suggested change

* @return the builder itself.

* @return The builder itself.

mgaido91 · 2020-04-16T20:05:03Z

+   * @param sessionId the id of the session to attach to,
+   *                  if not set a new session will be created when the client is built.


Suggested change

* @param sessionId the id of the session to attach to,

* if not set a new session will be created when the client is built.

* @param sessionId The ID of the session to attach to. If not set, a new

* session will be created when the client is built.

mgaido91 · 2020-04-16T20:06:00Z

+   * @return the builder itself.
+   */
+  public LivyClientBuilder setSessionId(int sessionId) {
+    config.setProperty(LIVY_SESSION_ID_KEY, "" + sessionId);


Suggested change

config.setProperty(LIVY_SESSION_ID_KEY, "" + sessionId);

config.setProperty(LIVY_SESSION_ID_KEY, String.valueOf(sessionId));

mgaido91 · 2020-04-16T20:07:29Z

-    Matcher m = Pattern.compile("(.*)" + LivyConnection.SESSIONS_URI + "/([0-9]+)")
-      .matcher(uri.getPath());
-
+    Matcher m = Pattern.compile("(.*)" + LivyConnection.SESSIONS_URI + "/([0-9]+)").matcher(uri.getPath());


why did you remove the comment above? moreover, here I see no changes, so please revert the change to keep the history clean

I did it because the in-line comment was the only doc about connecting to an existing session. Now that is documented in the builder I don't think the inline comment is still needed, anyway no problem at all at reverting that change.

mgaido91 · 2020-04-16T20:08:14Z

        URI base = new URI(uri.getScheme(), uri.getUserInfo(), uri.getHost(), uri.getPort(),
-          m.group(1), uri.getQuery(), uri.getFragment());
-
+            m.group(1), uri.getQuery(), uri.getFragment());


please revert unneeded change

tmnd1991 · 2020-04-16T20:47:07Z

@tmnd1991 you can't, JIRAs are assigned when related patches are merged and they are resolved

Then I think contributing section is wrong :)

…ient and add a property to the builder

tmnd1991 · 2020-04-16T21:02:10Z

I changed what you suggested @mgaido91 , thanks for the review.
Tagging @ajbozarth as he asked on Jira :)

ajbozarth · 2020-04-16T23:07:23Z

@mgaido91 I think your mixing up how Spark handles JIRA assignments with Livy's policy. We assign JIRAs once a PR is open for the issue. For some reason though, I can't assign issues to @tmnd1991 in the Livy JIRA at the moment. We had this issue before but I can't remember how we solved it.

@tmnd1991 A heads up, when you force push updates to commits it makes it difficult to track your changes as you address review (eg. I can't tell which of @mgaido91 comments you've addressed because there's no history to compare to). Since we squash all commits in a PR before merging the quantity of commits in a PR isn't an issue.

As for your code, I read through it and I don't see any issues with the current code, but I don't have to time check it out and more thoroughly test it so I will defer to @mgaido91 on final approval for this PR.

tmnd1991 · 2020-04-17T08:38:21Z

@tmnd1991 A heads up, when you force push updates to commits it makes it difficult to track your changes as you address review (eg. I can't tell which of @mgaido91 comments you've addressed because there's no history to compare to). Since we squash all commits in a PR before merging the quantity of commits in a PR isn't an issue.

Good, I did not know if the squash before merge was a "thing" in Livy repo, I won't do the same mistake again :)

As for your code, I read through it and I don't see any issues with the current code, but I don't have to time check it out and more thoroughly test it so I will defer to @mgaido91 on final approval for this PR.

👍

mgaido91 · 2020-04-17T20:51:35Z

@mgaido91 I think your mixing up how Spark handles JIRA assignments with Livy's policy. We assign JIRAs once a PR is open for the issue. For some reason though, I can't assign issues to @tmnd1991 in the Livy JIRA at the moment. We had this issue before but I can't remember how we solved it.

Sorry, I saw the same pattern being followed in Livy too in practice so I assumed it was the same. I think the point is that @tmnd1991 has to be added to the contributors in JIRA. I'll do that when resolving the JIRA.

mgaido91

LGTM apart from some wordings.

Co-Authored-By: Marco Gaido <marcogaido91@gmail.com>

mgaido91 · 2020-04-20T09:46:45Z


+  /**
+   * Sets the URI of the Livy server the client will connect to. If the URI contains
+   * <pre>sessions/{sessionId}</pre>, the client will connect to the specified existing session;


one last question: @ajbozarth @jerryshao @tmnd1991 I think that by adding this API, we might want to deprecate this "hacky" approach and eventually remove the support to it in next major release. Any thought on this?

I 100% agree on removing the hacky (and until now not even documented) approach. I left it to make the PR backward compatible

yes, we definitely need to leave it for now, but we can start deprecating it. Let's wait for others opinion .

codecov-commenter · 2026-04-29T19:04:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.36%. Comparing base (ee7fdfc) to head (185b351).
⚠️ Report is 84 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #290      +/-   ##
============================================
+ Coverage     68.19%   68.36%   +0.16%     
- Complexity      964      970       +6     
============================================
  Files           104      104              
  Lines          5952     5961       +9     
  Branches        900      902       +2     
============================================
+ Hits           4059     4075      +16     
+ Misses         1314     1306       -8     
- Partials        579      580       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mgaido91 reviewed Apr 16, 2020

View reviewed changes

tmnd1991 force-pushed the feature/LIVY-758 branch from 09cd522 to 8659ab9 Compare April 16, 2020 20:55

[LIVY-758] Document how to attach to an existing session from Java cl…

682cec4

…ient and add a property to the builder

tmnd1991 force-pushed the feature/LIVY-758 branch from 8659ab9 to 682cec4 Compare April 16, 2020 20:56

mgaido91 reviewed Apr 17, 2020

View reviewed changes

tmnd1991 and others added 5 commits April 17, 2020 22:57

Update api/src/main/java/org/apache/livy/LivyClientBuilder.java

60137e2

Co-Authored-By: Marco Gaido <marcogaido91@gmail.com>

Update api/src/main/java/org/apache/livy/LivyClientBuilder.java

84a81b4

Co-Authored-By: Marco Gaido <marcogaido91@gmail.com>

Update api/src/main/java/org/apache/livy/LivyClientBuilder.java

04854b2

Co-Authored-By: Marco Gaido <marcogaido91@gmail.com>

Update api/src/main/java/org/apache/livy/LivyClientBuilder.java

2ebdbb3

Co-Authored-By: Marco Gaido <marcogaido91@gmail.com>

[LIVY-758] Wrap docs to 100 chars

185b351

mgaido91 reviewed Apr 20, 2020

View reviewed changes

		* @param sessionId the id of the session to attach to,
		* if not set a new session will be created when the client is built.

	config.setProperty(LIVY_SESSION_ID_KEY, "" + sessionId);
	config.setProperty(LIVY_SESSION_ID_KEY, String.valueOf(sessionId));

Conversation

tmnd1991 commented Apr 16, 2020

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

tmnd1991 commented Apr 16, 2020

Uh oh!

mgaido91 commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

tmnd1991 commented Apr 16, 2020

Uh oh!

tmnd1991 commented Apr 16, 2020

Uh oh!

ajbozarth commented Apr 16, 2020

Uh oh!

tmnd1991 commented Apr 17, 2020

Uh oh!

mgaido91 commented Apr 17, 2020

Uh oh!

mgaido91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

codecov-commenter commented Apr 29, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants