Skip to content

Conversation

@szehon-ho
Copy link
Member

@szehon-ho szehon-ho commented Aug 12, 2023

This was added in #3651, and I would like to propose its removal. We no longer have Spark 2, which was the only reason Java 8 was set here.

This is actually now the wrong default for jenv users.

@szehon-ho
Copy link
Member Author

@findepi @nastra @RussellSpitzer does it make sense?

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok removing this because I don't use it, but let's see what @findepi thinks

@findepi
Copy link
Member

findepi commented Aug 14, 2023

.java-version is used by jenv to pick the right version for particular project. I don't know any other convention or tooling that would solve this mundane problem of choosing the right JDK, which might be a challenge for people who work on different projects having different minimum JDK versions. I do recommend having such a file (with correct contents of course), but it's up to project maintainers to decide. After all the maintainers choose whether they maintain or drop a

@szehon-ho
Copy link
Member Author

In my thought, as build.gradle supports building of all Java versions, it could be a surprise to jenv users that Iceberg overrides the user's global default. Also i took a brief look at some other projects like Parquet/Spark/Flink/Hive, and didn't see .java-version there. I think this made sense before when Spark 2 was not built without java8, but now that has been removed and don't see a strong reason to default Iceberg to a specific java version overriding the user's default. That being said, I can leave this a little bit and see if anyone has strong comments against its removal.

@aokolnychyi
Copy link
Contributor

Are there any other modules that are only compiled using JDK 8? Hive? Is that even being used?

I guess the primary benefit of having this file was to force building of all modules, which is helpful to catch compile-time errors after changes in core. Given that we changed our deprecation model and added support for quite a bit of Java versions at this point, I see how it may be surprising to have this override. I am OK either way but we need to double check our release scripts to make sure they have proper validation. We still have to use Java 8 there.

@szehon-ho
Copy link
Member Author

I think this file is for jenv users only. There is a explicit check for java version in release: https://github.com/apache/iceberg/blob/master/deploy.gradle#L21 so we should be fine there as I can see.

Copy link
Contributor

@dramaticlly dramaticlly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I use jenv for building iceberg locally and I dont think we need to dictate on java version here

@szehon-ho szehon-ho merged commit da127a4 into apache:master Aug 22, 2023
@szehon-ho
Copy link
Member Author

szehon-ho commented Aug 22, 2023

Merging, I think there's no strong objection. If we have some reason in future to must build with a certain Java version, we can put this back. For now, as Iceberg can be built in many distro using different Java version, this allows more flexibility. Thanks everyone for discussions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants