-
Notifications
You must be signed in to change notification settings - Fork 29k
Update JavaSparkContextVarargsWorkaround.java #14402
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
Taking a common functionality in one method to avoid duplication of code.
Update JavaSparkContextVarargsWorkaround.java
| return union(rdds[0], rest); | ||
| } | ||
|
|
||
| public final List<T> populateRDDList(T rdds) { |
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.
OK, pretty trivial but a win. You can use this in line 49 too right? and this can be static
Update JavaSparkContextVarargsWorkaround.java
Update JavaSparkContextVarargsWorkaround.java
| return union(rdds[0], rest); | ||
| } | ||
|
|
||
| public static final List<T> populateRDDList(T rdds) { |
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.
One last thing -- private? And doesn't this have to be T... rdds to work?
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 agree with making the method "private".But rdds is not any varargs so why do we need ...??
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.
This doesn't build. You're using T like an array type but it isn't. It has to be an array type as far as the method is concerned. T[] would be fine as the type.
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 created a build with T...
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.
will this work ??
Update JavaSparkContextVarargsWorkaround.java
Making method private and T as an array [].
Update JavaSparkContextVarargsWorkaround.java
|
Any update on the recent change ?? |
|
Jenkins test this please |
|
Test build #63042 has finished for PR 14402 at commit
|
|
Yeah, still doesn't compile. Are you compiling this locally? |
Update JavaSparkContextVarargsWorkaround.java
|
Added the required fix.Can you please start the build ?? |
|
@sakky11 you have not even addressed the compilation error. This is wasting everyone's time. Please compile this locally and fix it before pinging again. |
|
Hey sorry @srowen ,Actually I have fixed the compilation error but missed it in adding in a pull request.Sorry. |
Type Parameter <T>
Update JavaSparkContextVarargsWorkaround.java
|
I think the issue should be fixed now.Sorry for trouble. |
|
Did you compile this locally before pushing? |
|
This patch is only a one-line reduction of code and touches a file which hasn't needed to be modified in ages; is this change really necessary / worth spending review time on? |
|
I know its trivial but it still avoids duplication of code and make it more readable. |
|
Any update on this commit ?? |
|
@sakky11 I asked you because I can already see that your change doesn't even compile, for a third time. This is a trivial change, and not worth spending more time discussing. Please close this, and read https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark before opening another change. At minimum, you need to make sure your change even builds before considering opening a pull request. |
Taking common functionality into a Single method to avoid duplication of code.