-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HWORKS-1416] Close sparks sessions #1370
[HWORKS-1416] Close sparks sessions #1370
Conversation
suceeded = true; | ||
} finally { | ||
LOGGER.info("Closing spark session..."); | ||
engine.closeSparkSession(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this code outside the if statement considering that if an exception happens, the job shuts down correctly.
On the other hand, if by any chance the op is not one of those 2s, the if and the finally will be skipped entirely not closing the session.
public void closeSparkSession() { | ||
if (getSparkSession() != null) { | ||
try { | ||
getSparkSession().sparkContext().stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you calling stop()
on the spark context and not close()
on the Spark session?
The spark context is a "component" of the spark session. You want to close the entire session, not a single component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Python side you are correctly closing the session and not just the context.
} catch (Exception e) { | ||
// No-OP | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why catching the exception in the first place? If something happens in the .stop()
it would be useful if it shows up in the logs, so I would say we should avoid swallowing the exception.
utils/python/hsfs_utils.py
Outdated
if spark is not None: | ||
try: | ||
spark.stop() | ||
except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
This PR adds/fixes/changes...
JIRA Issue: -
Priority for Review: -
Related PRs: -
How Has This Been Tested?
Checklist For The Assigned Reviewer: