Enforce JDK 11 for development, but build Ingestion to target Java 8#518
Conversation
ches
requested review from
davidheryanto,
khorshuheng,
pradithya,
woop and
zhilingc
as code owners
|
/assign @davidheryanto |
|
Failure on |
|
Thanks for this @ches. I've commented on the issue. Happy to lgtm/approve if you are done. |
ches
force-pushed
the
517-enforce-java-11-for-dev
branch
from
2ee933b to
5ab68b7
Compare
|
/test test-end-to-end-batch |
Functionally this is working for me, with light experimentation. Assuming agreement on #517, I don't feel there's anything more to do on this branch except maybe add a note similar to the last part of #517 (comment) to our development docs to clearly explain the policy and rationale. It doesn't seem for me that IntelliJ automatically picked up the different |
|
@ches: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@ches would you mind rebasing this one? I don't think the tests will pass otherwise. |
We build for Java 11 now, so the build will fail with older JDKs. Have the Enforcer plugin do that for a more lucid error message. This reverts 190e605 which was squash-merged in a larger commit. Partially addresses feast-dev#517
As well as datatypes-java since ingestion depends on it. Java 11 is desirable for the other components, but for Beam it may impose limitations on what runners Feast can support, if it is even safe to run on an 11 JRE now. https://issues.apache.org/jira/browse/BEAM-2530 References feast-dev#517
ches
force-pushed
the
517-enforce-java-11-for-dev
branch
from
327835e to
c6d768c
Compare
|
Done, hadn't responded to the bot yet because I wanted to come back to adding the development guide info anyway. Also done now. |
|
|
||
| #### 5.2 IntelliJ Tips and Troubleshooting | ||
|
|
||
| For IntelliJ users, this section collects notes and setup recommendations for working comfortably on the Feast project, especially to coexist as peacefully as possible with the Maven build. |
There was a problem hiding this comment.
I'll move some other IntelliJ advice here that's duplicated (and out of sync) between the Contributing and Development guides, in another PR or GitBook edit.
I believe the thinking was that Contributing was getting unwieldy with technical setup / dev environment verification—I would agree, will take a look at moving things over with references left in Contributing. Probably this is also a legacy of duplicate CONTRIBUTING.md kept in the root awhile for GitHub.
There was a problem hiding this comment.
Yip, your assessment is correct. Lot's of little pebbles here, as you call them. We can chip away at it over time.
|
/approved |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ches, woop The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
With the addition of the Storage APIs and connector implementations, we did not specify that their build should target Java 8 bytecode. Because Ingestion depends on them, we regressed on keeping Ingestion compatible with Java 8 runtimes according to feast-dev#518. This could be confirmed prior to this change with, for example: $ mvn -pl ingestion clean compile $ javap -v storage/connectors/redis/target/classes/feast/storage/connectors/redis/writer/RedisCustomIO.class | grep major major version: 55 I take this as proof that a blacklist approach with 11 as the default in our root POM is error prone, so this change flips it so that 8 is default and our leaf applications that are able use 11 opt into it in whitelist fashion.
…es (#722) * Make storage modules target Java 8, since Ingestion must With the addition of the Storage APIs and connector implementations, we did not specify that their build should target Java 8 bytecode. Because Ingestion depends on them, we regressed on keeping Ingestion compatible with Java 8 runtimes according to #518. This could be confirmed prior to this change with, for example: $ mvn -pl ingestion clean compile $ javap -v storage/connectors/redis/target/classes/feast/storage/connectors/redis/writer/RedisCustomIO.class | grep major major version: 55 I take this as proof that a blacklist approach with 11 as the default in our root POM is error prone, so this change flips it so that 8 is default and our leaf applications that are able use 11 opt into it in whitelist fashion. * Target Java 8 compatibility for Java SDK
4 participants
What this PR does / why we need it:
This is a PoC for the idea in #517 that Ingestion could be easily built for Java 8 while other components continue to target 11.
Which issue(s) this PR fixes:
Addresses #517, speculatively and not completely.
Does this PR introduce a user-facing change?: