Restore Feast Java SDK and Ingestion compatibility with Java 8 runtimes #722
Conversation
ches
added
kind/bug
backport-candidate
ches
requested review from
davidheryanto,
khorshuheng,
pradithya,
woop and
zhilingc
as code owners
| We want to keep Ingestion compatible with Java 8 runtime for Beam runners, #518. | ||
| Unfortunately this doesn't seem to work on Reactor modules, though: | ||
| https://github.com/mojohaus/mojo-parent/issues/85 |
There was a problem hiding this comment.
I hoped that this would fail before applying the fix, verifying that it could catch this problem, but it didn't. I kept this in, because if it does fail on third-party dependencies, we would probably like to know if/when it changes in the future. This rule is from extra-enforcer-rules which was added #451 but not actually used for anything so far, I think.
|
/hold Tests need compilation fixes |
|
/retest |
|
/hold cancel |
|
/retest |
|
/assign @zhilingc |
sdk/java/pom.xml
Outdated
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <configuration> | ||
| <release>11</release> |
There was a problem hiding this comment.
Somewhat of a question whether SDK should build for 8 or 11 perhaps. It seems like the ecosystem still widely supports 8 for libraries. Here it was implicitly 11 since that became default in the root, so I preserved that, but I can also change it now if there is a desire to. Needs a small code change in tests I think.
There was a problem hiding this comment.
What a timely comment - @khorshuheng just ran into this during our upgrade today :')
There was a problem hiding this comment.
I'll take that as a real-world indicator that yes, we should keep it Java 8-compatible. Pushed this change.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ches The full list of commands accepted by this bot can be found here.
Needs 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.
|
Rebased so #725 should fix the build. |
|
/retest |
ches
changed the title
ches
changed the title
|
It's not clear why these e2e tests are failing inside Docker Compose, yet they work when run outside of it. I have removed the from PRs and allow them only to run on master for the time being. Unfortunately I cant remove it from the list of checks, so we probably have to force merge once you are ready. Created to track the issue #726 |
This is ready from my side, if it makes sense to everyone else. Ditto for #721 blocked by the misbehaved check. |
ches
added
area/ingestion
What this PR does / why we need it:
Restores Feast Ingestion compatibility with Java 8 runtimes.
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 and bundles them in an uber jar, we regressed on keeping Ingestion compatible with Java 8 runtimes according to #518.
This could be confirmed prior to this change on master with, for example:
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.
This should be backported to
v0.5-branch, I believe.Which issue(s) this PR fixes:
None, perhaps should have filed a bug separately, but this can be bug and fix in one☺️
Does this PR introduce a user-facing change?: