-
Notifications
You must be signed in to change notification settings - Fork 224
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
TOREE-387: Always use SparkSession.builder.getOrCreate. #103
TOREE-387: Always use SparkSession.builder.getOrCreate. #103
Conversation
LGTM. @rdblue, would you mind rebasing? Once you do, I'll merge this in. |
Keeping a reference to a SparkSession prevents users from restarting the Spark application in long-running notebooks.
73416ff
to
a739674
Compare
@chipsenkbeil, I've rebased and will watch to see if there are test issues. Thanks for reviewing this! If you have a chance to look at my other small pull requests (not #104, that's not ready to merge), I'd appreciate that as well. They're useful changes that were blockers for us moving to tests with users. |
@rdblue, no problem. I'll take a look at the other ones today. If possible, I'd need you to rebase those as I prepare to merge them. This allows our Apache bot to mark the PR as merged. We don't have administrative access over the repository; so, we cannot merge/close PRs on our own. I'll mention you on each PR as I go through them. |
Thanks @chipsenkbeil! You might have more flexibility than you'd think with the integration between Apache infra and github. I've pushed commits to Avro and had it correctly detected that it was a merge. I'm not sure if that had to be a fast-forward or not, but I thought it didn't. You can also close a PR by adding "closes #103" to the commit message. That's how our merge script for Parquet works (which we took from Spark). It squashes all of the commits from the PR and builds a commit message that will close it. |
I forgot to mention: I'm happy to rebase my PRs, just let me know which one you want to look at next. |
@rdblue, thanks for the tip about closing! I tried it before, but must have done something wrong because it didn't close the PR. As for merging, we've been told to rebase as much as possible to keep a clean commit log. Maybe a merge commit would aid in closing the PR? Regardless, if you're able to rebase, that'll be a big help! |
Travis seems to be doing nothing at the moment. I'd rather get these items in and repair any damage with additional commits later. So, going to merge. |
A merge commit might work, but I dislike using them. I'd much rather have rebases than merges since it is so much easier to revert. I think the question is whether the integration will pick up on cherry-picked commits or rebases on your end, or just fast-forward. That's how I merge everything in Avro, but I often forget to add the bit to close PRs. |
@rdblue, that I've seen, the integration does not pick up on rebases that I conduct myself. I don't know about cherry-picked commits. |
Keeping a reference to a SparkSession prevents users from restarting the Spark application in long-running notebooks. This update the kernel so that it always returns the current Spark session from
SparkSession.builder.getOrCreate
.This also adds a cache of
JavaSparkContext
to avoid creating one every timejavaSparkContext
is called. The cache uses weak keys, so the entries disappear when the Spark session is garbage collected.