[SPARK-25408] Move to mode ideomatic Java8 by Fokko · Pull Request #22399 · apache/spark

Fokko · 2018-09-11T20:45:29Z

While working on another PR, I noticed that there is quite some legacy Java in there that can be beautified. For example the use og features from Java8, such as:

Collection libraries
Try-with-resource blocks

No code has been changed

What are your thoughts on this?

This makes code easier to read, and using try-with-resource makes is less likely to forget to close something.

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Please review http://spark.apache.org/contributing.html before opening a pull request.

AmplabJenkins · 2018-09-11T20:47:56Z

Can one of the admins verify this patch?

TomaszGaweda · 2018-09-11T21:12:14Z

If you are already changing this, I would also suggest to add static imports to shorten the code

I don't think this sort of thing is worth changing. I know, IntelliJ suggests it. Unless the mutability is an issue, I'd leave it as a shorter idiom. I wouldn't use static imports here personally.
there's also a very small cost of changes in that they create potential merge conflicts for other changes later, so I tend to have a very low but finite minimum bar for value of code scrubbing like this.

I agree, I don't really care about moving to Collections.

TomaszGaweda · 2018-09-11T21:16:59Z

I like this idea :) +1 as it's only refactor, without logic change

srowen · 2018-09-12T02:42:07Z

I don't think this sort of thing is worth changing. I know, IntelliJ suggests it. Unless the mutability is an issue, I'd leave it as a shorter idiom. I wouldn't use static imports here personally.
there's also a very small cost of changes in that they create potential merge conflicts for other changes later, so I tend to have a very low but finite minimum bar for value of code scrubbing like this.

srowen · 2018-09-12T02:42:23Z

Nit: space after try

srowen · 2018-09-12T02:43:32Z

Same comment about space above; I'd also indent the continuation 4 spaces for clarity

srowen · 2018-09-12T02:43:48Z

Indent is now too deep here. I have the same general kind of doubt here.. it's touching a lot of lines for little actual gain. Still, I'd like to be able to improve code a bit here and there. If this is only going to master and Spark 3, the back-port issue lessens, because it's more unlikely to backport from 3.x to 2.x.

If there are merge conflicts, it is easy to pick theirs, and wrap the try-with-resources around it.

The scenario is something like: later, a line or two in this block is changed. That change is cherry-picked to a branch before this change here. It's a merge conflict, and choosing "theirs" overwrites your change here. It would have to be manually merged. If you know that this is all that has happened, sure, not hard. But it relies on the committer figuring that out and not missing another subtle change during the manual merge. That's definitely the most time-consuming part for me. For Spark 3.x vs 2.x I am less worried, and would like a freer hand to make small improvements. Within a major release I am reluctant.

srowen · 2018-09-12T02:45:46Z

Don't collapse these

srowen · 2018-09-12T02:46:28Z

I wouldn't bother with this; we don't really do it consistently, it's not a JDK standard class, and it doesn't really save much, while adding to the dependency on commons/io

This module should not have any non-JRE dependencies.

I wan't aware of this requirement, reverted the change.

HyukjinKwon

was just looking through this out of curiosity. just few nits.

HyukjinKwon · 2018-09-12T07:49:54Z

Shell we stick to 2spaced indentation?

HyukjinKwon · 2018-09-12T07:50:04Z

ditto for indentation

HyukjinKwon · 2018-09-12T07:50:29Z

indentation ..

This one is horrible yes

HyukjinKwon · 2018-09-12T07:51:13Z

Looks unused.

kiszk · 2018-09-12T09:31:14Z

Would it be possible to add a space after try?

kiszk · 2018-09-12T09:31:33Z

kiszk · 2018-09-12T09:36:46Z

nit: fix indentation

kiszk · 2018-09-12T09:37:32Z

nit: space after try

kiszk · 2018-09-12T09:37:47Z

Fokko · 2018-09-12T09:45:08Z

Thanks for the feedback guys, I was relying too much on Scalastyle :)

kiszk · 2018-09-12T09:56:36Z

Do we need try at line 149?

Good one, thanks. Missed it for some reason.

Fokko · 2018-09-17T13:58:12Z

Addressed comments and rebased onto master, no merge conflicts.

kiszk · 2018-09-17T15:33:17Z

nit: Can we put this line into try since this line was originally executed after clientFactory.createClient.

No we can't, since the res is returned by the function :-)

I see, thanks.

Fokko · 2018-09-20T06:30:03Z

@srowen Any incentive to move this forward? Or are PR's like these not appreciated? Let me know.

Most of the changes are cosmetic, but having <Java8 in the codebase give a bad example to newcomers to the Spark project. Also adding the Closeable to the RowBasedKeyValueBatchSuite.java:
https://github.com/apache/spark/pull/22399/files#diff-6c2c45f79666e2e52eb9f9411fa8b4baR49
makes the codebase a bit nicer in my opinion, since the class already has a .close() method, it makes sense to also implement the Closeable interface.

srowen · 2018-09-20T10:07:11Z

I think this is fine (though see https://github.com/apache/spark/pull/22399/files#r218121426 ). As a practical matter I might wait until we're done with the 2.4 release to start a lot of changes like this. It isn't tied to Spark 2.5 or 3.0 but might just be simpler to hold this for a bit, while we might be wanting to quickly backport a few changes from master to 2.4.

Fokko · 2018-09-20T20:19:17Z

Cool, makes sense. Thanks for the clarification @srowen

Use features from Java8 such as: - Try-with-resource blocks

Fokko · 2018-10-04T06:13:19Z

Rebased onto master

srowen · 2018-10-05T01:58:42Z

Merged to master

gatorsmile · 2018-10-05T03:00:30Z

It sounds like the build is broken after merging this PR

gatorsmile · 2018-10-05T03:00:48Z

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Compile/job/spark-master-compile-maven-hadoop-2.6/8432/

gatorsmile · 2018-10-05T03:00:58Z

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Compile/job/spark-master-compile-maven-hadoop-2.7/8521/

cloud-fan · 2018-10-05T03:02:40Z

Jenkins was not triggered in this PR, I'm reverting it. Please re-open it and make sure jenkins pass, thanks!

srowen · 2018-10-05T03:31:24Z

Oh no, I got mixed up and didn't realize this one hadn't actually had a jenkins run. Sorry about this, that is my mistake. I will follow up.

srowen · 2018-10-05T03:33:37Z

Sorry @Fokko can you recreate this change in a new PR, after addressing...

[error] /home/jenkins/workspace/spark-master-compile-maven-hadoop-2.6/sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/CLIService.java:149: error: incompatible types: try-with-resources not applicable to variable type
[error] try (IMetaStoreClient metastoreClient = new HiveMetaStoreClient(hiveConf)) {
[error] ^
[error] (IMetaStoreClient cannot be converted to AutoCloseable)
[error] /home/jenkins/workspace/spark-master-compile-maven-hadoop-2.6/sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/operation/OperationManager.java:200: error: incompatible types: try-with-resources not applicable to variable type
[error] try (Operation operation = removeOperation(opHandle)) {
[error] ^
[error] (Operation cannot be converted to AutoCloseable)

Fokko · 2018-10-05T04:16:27Z

Thanks @srowen for pointing out the errors. Weird that it did not come up as a merge conflict. Let me open a new PR.

srowen · 2018-10-05T04:24:35Z

It wasn't a merge conflict. I failed to notice this had not actually been tested. To check locally you'd have to make sure you enable more profiles like Hive to build some of the code that changed here.

HyukjinKwon · 2018-10-05T04:25:02Z

@Fokko, Let's follow the PR format next time BTW, for instance, "How was this patch tested?"

Fokko · 2018-10-05T04:30:02Z

Oh I see, could we test the next PR using Jenkins?

HyukjinKwon · 2018-10-05T04:36:29Z

Let me trigger it in the next PR.

Fokko · 2018-10-05T05:00:56Z

@HyukjinKwon I've opened a new PR under #22637. Would be nice if you can trigger Travis 👍

While working on another PR, I noticed that there is quite some legacy Java in there that can be beautified. For example the use og features from Java8, such as: - Collection libraries - Try-with-resource blocks No code has been changed What are your thoughts on this? This makes code easier to read, and using try-with-resource makes is less likely to forget to close something. ## What changes were proposed in this pull request? (Please fill in changes proposed in this fix) ## How was this patch tested? (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) (If this patch involves UI changes, please attach a screenshot; otherwise, remove this) Please review http://spark.apache.org/contributing.html before opening a pull request. Closesapache#22399 from Fokko/SPARK-25408. Authored-by: Fokko Driesprong <fokkodriesprong@godatadriven.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

TomaszGaweda reviewed Sep 11, 2018
View reviewed changes

srowen requested changes Sep 12, 2018
View reviewed changes

Fokko force-pushed the SPARK-25408 branch from d1df30d to 2314b6aCompare September 12, 2018 07:39

HyukjinKwon reviewed Sep 12, 2018
View reviewed changes