Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Java 11 #451

Merged
merged 2 commits into from
Feb 11, 2020
Merged

Upgrade to Java 11 #451

merged 2 commits into from
Feb 11, 2020

Conversation

Yanson
Copy link
Contributor

@Yanson Yanson commented Jan 30, 2020

What this PR does / why we need it:

Upgrade Feast to next LTS - Java 11 - since Java 8 is outdated and no longer supported.

Our internal build and testing of Feast has been on Java 11 all along, so it's fair to say there shouldn't be any problems. I'm not able to fully test because 1) slight variation in base-images for building/packaging, 2) we use a different Dockerfile that uses pre-built Jars from another step rather than a 'builder' pattern

Which issue(s) this PR fixes:
NONE

Does this PR introduce a user-facing change?:

Feast now requires a Java 11 runtime. JVM options should be set accordingly.

@feast-ci-bot
Copy link
Collaborator

Hi @Yanson. Thanks for your PR.

I'm waiting for a gojek member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

@Yanson
Copy link
Contributor Author

Yanson commented Jan 30, 2020

/assign @woop

@khorshuheng
Copy link
Collaborator

@Yanson One of the reason which prevented us from using Java 11, is due to Apache Beam / Dataflow does not officially supporting Java 11. So we certainly need to check if this change will affect ingestion, especially for Dataflow runner.

In any case there shouldn't be any issue running the serving component on Java 11.

@khorshuheng
Copy link
Collaborator

/ok-to-test

@woop
Copy link
Member

woop commented Jan 31, 2020

This change makes sense, but we'd need to ensure compatibility first before making the move like @khorshuheng said.

@Yanson
Copy link
Contributor Author

Yanson commented Jan 31, 2020

Looking at the test failures, it seems as though they are still running on maven:3.6-jdk-8

e.g. http://prow.feast.ai/prowjob?prowjob=9bf59c76-43cf-11ea-9ebf-e2420cc0b547

Is it possible to re-run with the .prow/config.yaml changes in this PR?
https://github.com/gojek/feast/pull/451/files#diff-7c2c7257959d7451ee7dc1de3fb9889e

@Yanson
Copy link
Contributor Author

Yanson commented Jan 31, 2020

@Yanson One of the reason which prevented us from using Java 11, is due to Apache Beam / Dataflow does not officially supporting Java 11. So we certainly need to check if this change will affect ingestion, especially for Dataflow runner.

In any case there shouldn't be any issue running the serving component on Java 11.

We are using Java 11 for all Dataflow projects internally, and as noted our Feast build is also on Java 11. The only compatibility problem is the way in which files to upload are detected.

Of course, I don't expect you to take my word for it so let's get it building on Prow as the first step.

@Yanson
Copy link
Contributor Author

Yanson commented Feb 3, 2020

@woop @khorshuheng I am not familiar with prow, is there something one of us needs to do to get this to build with newly specified base images?

@khorshuheng
Copy link
Collaborator

@woop @khorshuheng I am not familiar with prow, is there something one of us needs to do to get this to build with newly specified base images?

By design, prow config change will only happen after the change has been merged into master. But this PR happens to be an exception where this design become unsuitable.

We will be testing this on our end first, by replicating the prow test on our environment.

@woop
Copy link
Member

woop commented Feb 9, 2020

@Yanson Thank you for your patience. Will be having a look at this soon.

@woop
Copy link
Member

woop commented Feb 11, 2020

/retest

1 similar comment
@woop
Copy link
Member

woop commented Feb 11, 2020

/retest

@woop
Copy link
Member

woop commented Feb 11, 2020

/test test-end-to-end-batch

@woop
Copy link
Member

woop commented Feb 11, 2020

/lgtm
/approve

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: woop, Yanson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feast-ci-bot feast-ci-bot merged commit 8e227d5 into feast-dev:master Feb 11, 2020
@feast-ci-bot
Copy link
Collaborator

@Yanson: Updated the config configmap in namespace default at cluster default using the following files:

  • key config.yaml using file .prow/config.yaml

In response to this:

What this PR does / why we need it:

Upgrade Feast to next LTS - Java 11 - since Java 8 is outdated and no longer supported.

Our internal build and testing of Feast has been on Java 11 all along, so it's fair to say there shouldn't be any problems. I'm not able to fully test because 1) slight variation in base-images for building/packaging, 2) we use a different Dockerfile that uses pre-built Jars from another step rather than a 'builder' pattern

Which issue(s) this PR fixes:
NONE

Does this PR introduce a user-facing change?:

Feast now requires a Java 11 runtime. JVM options should be set accordingly.

Instructions 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.

@ches
Copy link
Member

ches commented Mar 7, 2020

This is good for Core and Serving especially because of container runtime friendliness improvements that have come since Java 8, but I think it might be wise to continue building Ingestion with 8, given Beam support is still an open issue to-date.

This would be a bit annoying since the core module depends on ingestion, although that is something I feel ought to be fixed…

Internally we'll probably have to maintain building Ingestion that way for quite some time, as Spark is our Beam runner. It's likely to be awhile yet before Spark 3 (now at preview stage) supporting Java 11 makes it to enterprise Hadoop distributions, cloud platforms, etc.

I believe it's reasons like this that vendors are supporting OpenJDK 8 until at least 2023.

@Yanson
Copy link
Contributor Author

Yanson commented Mar 9, 2020

That's disappointing to hear. As mentioned, we don't have an issue with Java 11 and Dataflow, but will likely be looking at Spark soon so if that's a showstopper we will require the change too.

Unsure how we would go about multi-project maven build with different JDKs.

Do you have more details on how you're using Spark as a runner? (OT but please find a place to discuss, e.g. Slack)

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

Successfully merging this pull request may close these issues.

6 participants