-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-1477]: Add the lifecycle interface #991
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
Conversation
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15497/ |
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15552/ |
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.
shouldn't we declare this in scala?
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.
The reason for this is java code inherited this interface easier.
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.
Java code can actually extend Scala traits as well. Do you have other concerns?
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.
Done
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16156/ |
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16157/ |
|
Jenkins, retest this please. |
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16158/ |
|
QA tests have started for PR 991. This patch merges cleanly. |
|
QA tests have started for PR 991. This patch merges cleanly. |
|
QA results for PR 991: |
|
QA results for PR 991: |
|
QA tests have started for PR 991. This patch merges cleanly. |
|
QA results for PR 991: |
|
QA tests have started for PR 991. This patch merges cleanly. |
|
QA tests have started for PR 991. This patch merges cleanly. |
|
QA results for PR 991: |
|
Jenkins, retest this please. |
|
QA tests have started for PR 991. This patch merges cleanly. |
|
QA results for PR 991: |
|
@witgo I'm going to take a look at this later for 1.2. I think it's a good idea to have a Service abstraction for service that we can start/stop. The current API is slightly more complicated than necessary, but it is in good direction. |
|
QA tests have started for PR 991 at commit
|
|
QA tests have finished for PR 991 at commit
|
|
Hey @witgo I thought about this more, and at this point I'm not sure if it is worth it to standardize this interface. The reason is we have a lot of "services" in Spark, but they have different ways to be initialized (some of them would require having different initialize methods that might not fit into the LifeCycle trait. I'm also worried about the use of mixins with multiple traits having default implementations. In the past that has caused trouble in Java interoperability and binary compatibility. |
|
Maybe we should just implement Closeable? |
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.
@rxin Here has inherited Closeable class
|
I also don't think we should add extra interfaces that we aren't going to use. In Spark we never interact with these components in a generic way, so I don't see any value to having this extra cruft all over the place. |
|
Let's close this issue for now given the comments from me and @rxin. |
No description provided.