To cross build, or not to cross build Ingestion

[ches]

In #451 we upgraded outright to Java 11 for master / Feast v0.5+. Subsequently in #473 we restored support for v0.3 and v0.4 maintenance branches to be tested on CI with Java 8 (thanks!). I believe that has been broken by later updates to CI scripts, I'll consider that part of this issue.

The upgrade could pose problems for Feast's Beam-based Ingestion component. Beam itself does not build with Java 11 yet, and the scope of that issue is only the DirectRunner. Other runners could lag further behind, as for example Spark does not build or run on Java 11 until its version 3.0, which is in preview. Thus I presume Beam's SparkRunner will not build with Java 11 until a GM Spark release can.

Feast does not support any runners other than GC Dataflow (and DirectRunner), so there may be no immediate issue if these are working so far. But, supporting additional runners—if anticipated (#362)—could be blocked if Ingestion does not remain Java 8-compatible.

I wanted to raise the question of whether we should (cross?) build Ingestion with Java 8 to ensure it remains compatible for awhile longer.

The bug report that follows applies to current status quo, regardless of the question above.

Expected Behavior

1. Feast master gives a helpful error about Java requirement if a developer attempts to build with an older version than is supported.
2. Feast v0.3-branch and v0.4-branch can build & test on CI with Java 8. (Appears fixed by Relax OS check when starting e2e test #523 🎉)

Current Behavior

$ jenv version
openjdk64-1.8.0.242 (set by /Users/cmartin/.jenv/version)
$ mvn compile
<SNIP>
[ERROR] Fatal error compiling: invalid flag: --release

This stems from:

https://github.com/gojek/feast/blob/7633912cd8209cece2a21ebe344600342ef1ff9b/pom.xml#L408

because the flag was added to javac in Java 9. Maven Enforcer will catch this, proposed below.

For the second problem, CI fails with:

.prow/scripts/test-end-to-end-batch.sh only supports Debian stretch.
Please change your operating system to use this script.

Steps to reproduce

1. As above demo
2. Submit a PR to v0.3-branch as in v0.3 backport: Fail Spotless check before tests #516

Possible Solution

The unhelpful error from Maven would be improved by reverting 190e605, resulting in output like

[WARNING] Rule 1: org.apache.maven.plugins.enforcer.RequireJavaVersion failed with message:
Detected JDK Version: 1.8.0-242 is not in the allowed range [11.0,).

[ERROR] Some Enforcer rules have failed. Look above for specific messages explaining why the rule failed.

For the CI environment problem, restore compatibility of the test scripts with maven:3.6-jdk-8 Docker image. Prow always builds using master versions of scripts & config, I think? Ideally then, ensure a build is run on the maintenance branches when Prow scripts are changed, for as long as those Feast versions are supported.

For the Java 8 matter, a quick test seems to tell me that maven-compiler-plugin can compile select modules in a Reactor build with <release>8</release> while the others compile with 11. So I'm suggesting that it could be straightforward to only build Ingestion for 8, instead of cross-building for two versions and having a more complicated CI matrix. Note then that datatypes-java ought to also build with 8 to be safe.

[woop]

I wanted to raise the question of whether we should (cross?) build Ingestion with Java 8 to ensure it remains compatible for awhile longer.

only build Ingestion for 8, instead of cross-building for two versions and having a more complicated CI matrix. Note then that datatypes-java ought to also build with 8 to be safe.

As you rightly pointed out both Beam and Spark require Java 8 at the moment. I am happy to build datatypes + ingestion with 8 and the rest with 11, as long as this doesn't cause more problems. If supporting multiple languages is too much of a pain then reverting to purely 8 is still an option and shouldn't be hard to do.

Prow always builds using master versions of scripts & config, I think?

Correct, Prow only has one active configuration at a time, but it is able to have separate configurations per branch/tag.

[ches]

If supporting multiple languages is too much of a pain then reverting to purely 8 is still an option and shouldn't be hard to do.

#518 seems to work with very little effort, though I've experimented minimally so far. If that approach does prove sound, I think it's a nice compromise: cross-building any or all components for both versions seems unjustified, but it is nice to be able to use newer language improvements in Core and Serving at least.

Working with a newer JDK that has the --release flag lends confidence, as a common problem with the old -source and -target options was you could target an old bytecode version but still depend on newer platform APIs that wouldn't be available on the target JRE. --release seems to have been added to address precisely that pitfall in a simple way.

Maybe we can raise the question for next community call whether anyone is still limited to Java 8 JREs in their production deployment environments. The later container/Docker friendliness improvements are a strong motivator probably driving many orgs to upgrade if they haven't already, outside of specialized clusters like Hadoop.

Correct, Prow only has one active configuration at a time, but it is able to have separate configurations per branch/tag.

One of these days I'm going to digest the Prow docs more fully… Is the below a good idea, and does anyone have a strategy to suggest for it?

Ideally then, ensure a build is run on the maintenance branches when Prow scripts are changed, for as long as those Feast versions are supported.

I see the run_if_changed, branches, and skip_branches options for jobs, but I think these apply to building the branch where a code change happened. I'm wondering if a change in .prow/** on master could trigger builds of the v0.x-branch branches.

[ches]

@Yanson since #451 was yours, would this approach be a problem for how your org works?

TL;DR it is:

• Build Core and Serving with <release>11</release>
• Build Ingestion (and datatypes-java) with <release>8</release>

The latter to ensure Ingestion can continue to run on Java 8 runtimes, for Spark as Beam runner at least but also other general Beam uncertainty.

More generally, as things like #402 (comment) go forward, leaf application modules can target 11, shared library modules are constrained to 8 compatibility. From #518 it appears straightforward to maintain this.

[woop]

I see the run_if_changed, branches, and skip_branches options for jobs, but I think these apply to building the branch where a code change happened. I'm wondering if a change in .prow/** on master could trigger builds of the v0.x-branch branches.

I believe this is possible. @davidheryanto can you confirm? I believe we have full control once an event triggers.

[Yanson]

@Yanson since #451 was yours, would this approach be a problem for how your org works?

This sounds like a fair compromise for now. It won't cause us problems and I will update our cloudbuild.yaml as (or if) necessary.

[davidheryanto]

Yeah with run_if_changed we can filter what jobs should be run ONLY when certain files are modified.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ches]

With #518 and #722 I think we have moved on and can close this issue. When the ecosystem appears ready for libraries to require, and for the heavy runtimes of Beam runners to fully support, an LTS Java runtime beyond 8, we can revisit.

While running on Java 11 runtimes has advantages to recommend it, we haven't heard arguments for compelling gains from building Java 11 bytecode, so I don't see a justification for cross-compiling two versions of components like Feast Ingestion. For Core and Serving at least, we can enjoy developer comforts like type inference, new additions to the standard library, etc. Modules may earn more merit as time goes on.