NIFI-6175 Spark Livy - Improving Livy by patricker · Pull Request #3511 · apache/nifi

patricker · 2019-06-03T15:29:17Z

Description of PR

The Livy Session Controller is missing many of the options available, and many of them I feel are critical for this service to be useful (queue? conf? num of executors?)

Add in functionality to shutdown open sessions when service is disabled.

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

not-for-me

@patricker, I just left some comments as a one of suggestion.
Please consisder about them.

not-for-me · 2019-11-04T02:01:59Z

+    String APPLICATION_JSON = "application/json";
+    String USER = "nifi";
+
+    LivyBatch getBatchSession(Integer batchid) throws SessionManagerException;


How about renaming batchid as a camelCase?

including the below batchids :)

not-for-me · 2019-11-04T02:04:51Z

+        appendValue(payload, "className", className);
+
+        try {
+            if (codeArgs != null) {


I'm curious why you didn't use appendValue method in this try-catch block

Because they aren't values, they are arrays. Note the use of splitCollect instead.

not-for-me · 2019-11-04T02:07:30Z

+        }
+    }
+
+    private String splitCollect(String arg) throws IOException {


It would be nicer to rename transformCsvToJsonArrayStr.
Because It does two actions: split and, converting.

Thanks for the feedback, but it is supposed to do two things :)
In the UI, NiFi users provide a comma delimited list of values. In the code, we accept that comma delimited string and need to convert it to JSON. This method handles that.

not-for-me · 2019-11-04T02:11:38Z

+                //  Two scenarios: we have no elastic pool sizing, in which case definitely look for candidates
+                //  Or, we do have elastic pool sizing, in which case we need to make sure we are above our max
+                //      pool size
+                if(idleSessions > 0 && numSessions > sessionPoolSize && (!elasticSessionPool || numSessions > maxSessionPoolSize)) {


How about use idleSessionInfo.size() instead of idleSessions int value?

int idleSessions = idleSessionInfo.size(); if(idleSessions > 0 && numSessions > sessionPoolSize && (!elasticSessionPool || numSessions > maxSessionPoolSize)) { int sessionID = idleSessionInfo.keySet().stream().findFirst().get(); ...

Yes, the variable serves no purpose anymore. It's just leftover from the original code.

not-for-me · 2019-11-04T02:36:02Z

+                //  Two scenarios: we have no elastic pool sizing, in which case definitely look for candidates
+                //  Or, we do have elastic pool sizing, in which case we need to make sure we are above our max
+                //      pool size
+                if(idleSessions > 0 && numSessions > sessionPoolSize && (!elasticSessionPool || numSessions > maxSessionPoolSize)) {


I'm not sure Is this conditional statement satisfies all possible cases...

e.g.) idleSessions=2, numSessions=6, elasticSessionPool=true maxSessionPoolSize=4
=> we have to delete two idle sessions, but it only picks randomly one idle session and deletes it.

It only stops 1 idle session at a time, that is true, but on the next run through, which happens every few seconds, it will stop the next one, and the next one, until it is back to where it is supposed to be.

Thanks for your elaborate explanation.
I got your point.

not-for-me · 2019-11-04T02:39:21Z

HI, @patricker , @mattyb149
Is there any updates?...
I hope that this change would be merged soon. :)
Thanks @patricker for the contribution

…ndles open.

patricker · 2020-01-24T17:38:24Z

Rebased against 1.12-SNAPSHOT master.

@not-for-me I don't have any control when this will get merged. When a technology is not widely used, such as Livy, or in my case MS SQL Server CDC (another PR I've had in progress since late 2017), it can take a really long time before a team member comes along who is comfortable with testing and merging the code.

joewitt · 2020-01-24T17:41:42Z

@patricker if this goes fine in travis and you're confident it doesn't break backward compatibility and the L&N is covered ping me and I'll merge.

For new stuff being included we should keep the high bar. For improving things already in the kit we should be faster. Especially when it is a committer/PMC member doing it!

patricker · 2020-01-24T17:50:29Z

Thanks @joewitt. There are no changes to the pom, no new dependencies, so we are good on L&N with no changes.

Let me think about the, "confident it doesn't break backwards compatibility". I'm 95% sure, but I will review the code again, as it's been a while since I looked at the code currently in master.

naru014 · 2020-06-05T10:45:45Z

@patricker Can the contents in this PR be used to build the livy session controller and sparkinteractive processor? I have a use case to submit pyspark jobs from nifi. But the controllerservice and processor that comes along with nifi has limitations.

patricker · 2020-06-11T14:27:29Z

@naru014 Yes, it can. It also includes Batch processing controller service/processor. The one that comes with NiFi currently has severe limitations. I keep forgetting to get this one merged. I just need to update it for the latest version of NiFI.

naru014 · 2020-06-16T06:40:26Z

@naru014 Yes, it can. It also includes Batch processing controller service/processor. The one that comes with NiFi currently has severe limitations. I keep forgetting to get this one merged. I just need to update it for the latest version of NiFI.

@patricker Thank you. We have got this built and included in the nifi. But when using a livy session controller service per nifi flow, the flows in some cases dont use the session created by the controller for that flow, but any other available session. Is this expected? So currently instead of creating a livy session controller per flow, I have created one livy session controller on the parent nifi-flow and it is inherited by all the child flows. By doing that, the nifi cluster has say min 2 sessions available and is used by any running flow based on its availability.
Please let me know if there is a better way to use it.

patricker · 2020-06-16T13:55:32Z

@naru014 Yes, this is expected. This is somewhat a limitation of Livy 0.5.0. Livy will not let you find sessions by name until v0.6.0. This version is available, but I don't have it in my environment.

There are two work arounds I've used.

Use different accounts to run the session controllers. The code does check to see if the username matches between the session it grabs from Livy and the configuration.
Use multiple Livy servers, we used two different name nodes and setup an instance on each.

If you have Livy 0.6.0 you could update the code to work with session names. This would easily work around your issue, each session in a session controller could be named session-1, session-2, etc...

Note: Livy never updated the documentation when they added this feature (apache/livy#48). But it looks like you just replace the sessionId with the name when making calls.

naru014 · 2020-06-22T04:38:51Z

@naru014 Yes, this is expected. This is somewhat a limitation of Livy 0.5.0. Livy will not let you find sessions by name until v0.6.0. This version is available, but I don't have it in my environment.

There are two work arounds I've used.

Use different accounts to run the session controllers. The code does check to see if the username matches between the session it grabs from Livy and the configuration.

Use multiple Livy servers, we used two different name nodes and setup an instance on each.

If you have Livy 0.6.0 you could update the code to work with session names. This would easily work around your issue, each session in a session controller could be named session-1, session-2, etc...

Note: Livy never updated the documentation when they added this feature (apache/incubator-livy#48). But it looks like you just replace the sessionId with the name when making calls.

@patricker Thank you.

github-actions · 2021-04-25T00:16:55Z

We're marking this PR as stale due to lack of updates in the past few months. If after another couple of weeks the stale label has not been removed this PR will be closed. This stale marker and eventual auto close does not indicate a judgement of the PR just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale tag. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours.

pvillard31 · 2021-05-14T09:11:03Z

@patricker - any chance you can rebase this against main/latest?

github-actions · 2021-09-12T00:08:06Z

We're marking this PR as stale due to lack of updates in the past few months. If after another couple of weeks the stale label has not been removed this PR will be closed. This stale marker and eventual auto close does not indicate a judgement of the PR just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale tag. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours.

pvillard31 · 2021-10-26T09:35:14Z

@patricker - I know this PR has been automatically closed but I'm currently playing with NiFi and Livy and if you want to re-open it and rebase it, happy to review/test this one.

patricker · 2021-10-26T17:52:07Z

Hey @pvillard31. I've got some bad news, I changed companies and I don't have access to a Hadoop environment, and even less a Livy environment :(. I'd still love to see this PR get rebased and merged, but I don't think I'm the one to do it anymore.

pvillard31 · 2021-10-26T17:57:43Z

Thanks for the feedback @patricker - will try to have a look if I get the chance.
Good luck / have fun at your new job - there is always some room for NiFi :)

mattyb149 · 2021-12-13T22:15:49Z

Closing in favor of #5601 as it incorporates these changes and continues forward

patricker changed the title ~~NIFI-6175~~ NIFI-6175 Spark Livy - Add Support for All Missing Session Features Jun 3, 2019

patricker requested a review from mattyb149 June 3, 2019 15:31

patricker force-pushed the NIFI-6175 branch from 1f360a3 to 873d4f6 Compare June 27, 2019 17:36

patricker force-pushed the NIFI-6175 branch from d331ee0 to c611db6 Compare July 22, 2019 14:07

patricker changed the title ~~NIFI-6175 Spark Livy - Add Support for All Missing Session Features~~ NIFI-6175 Spark Livy - Improving Livy Jul 22, 2019

patricker force-pushed the NIFI-6175 branch 2 times, most recently from 77ee45e to e351af3 Compare July 22, 2019 18:04

not-for-me reviewed Nov 4, 2019

View reviewed changes

patricker added 3 commits January 24, 2020 17:33

NIFI-6175 Improving Livy

125e82f

NIFI-6175 Improving Livy

8d38976

NIFI-6175 Resolve Livy HTTP Client not closing connections/leaving ha…

bc8d25e

…ndles open.

patricker force-pushed the NIFI-6175 branch from 02146ac to bc8d25e Compare January 24, 2020 17:37

NIFI-6175 Resolve Livy Checkstyle Violations

0a5eb17

github-actions Bot added the Stale label Apr 25, 2021

github-actions Bot closed this May 11, 2021

patricker reopened this May 13, 2021

github-actions Bot removed the Stale label May 15, 2021

github-actions Bot added the Stale label Sep 12, 2021

github-actions Bot closed this Sep 28, 2021

mattyb149 removed the Stale label Oct 26, 2021

mattyb149 reopened this Oct 26, 2021

mattyb149 mentioned this pull request Dec 13, 2021

NIFI-6175: Improving Livy support #5601

Closed

13 tasks

mattyb149 closed this Dec 15, 2021

Uh oh!

Conversation

patricker commented Jun 3, 2019

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

not-for-me 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!

not-for-me Nov 4, 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!

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!

patricker Nov 4, 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!

Choose a reason for hiding this comment

Uh oh!

not-for-me commented Nov 4, 2019

Uh oh!

patricker commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joewitt commented Jan 24, 2020

Uh oh!

patricker commented Jan 24, 2020

Uh oh!

naru014 commented Jun 5, 2020

Uh oh!

patricker commented Jun 11, 2020

Uh oh!

naru014 commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patricker commented Jun 16, 2020

Uh oh!

naru014 commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 25, 2021

Uh oh!

pvillard31 commented May 14, 2021

Uh oh!

github-actions Bot commented Sep 12, 2021

Uh oh!

pvillard31 commented Oct 26, 2021

Uh oh!

patricker commented Oct 26, 2021

Uh oh!

pvillard31 commented Oct 26, 2021

Uh oh!

mattyb149 commented Dec 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

not-for-me Nov 4, 2019 •

edited

Loading

patricker Nov 4, 2019 •

edited

Loading

patricker commented Jan 24, 2020 •

edited

Loading

naru014 commented Jun 16, 2020 •

edited

Loading

naru014 commented Jun 22, 2020 •

edited

Loading