Moving Feast Java back into main repo under java/ package #1997

adchia · 2021-11-04T20:00:21Z

What this PR does / why we need it:
This is almost a verbatim copy of feast java logic back into feast under java/. A couple of key changes:

the protos in datatypes have new symlinks to the protos/ base package
the "mvn" command (in Makefile and infra scripts) refers to a pom file in the java/ subdirectory
renaming of github workflows to be java_[workflow name]
removing of makefile definitions that reference non-existent scripts

In followup PRs, we intend to remove:

References to Spark ingestion
Feast Core (including authentication, credentials)
Out of date documentation (e.g. architecture diagrams)
Feast Serving references to Feast Core (e.g. CoreSpecService)
Feast Serving connectors that are not supported from Python (e.g. BigTable, Cassandra). These can be ported over from Feast Java when we re-introduce those in python (as plugins)

We also will consolidate GitHub actions into master vs release vs PR actions (though we need to verify the actions work first).

As far as releases will go. We will switch back to having one consistent version for Feast Java + Feast Python components. This will require we deprecate the old maven + DockerHub package names (e.g. on https://hub.docker.com/u/feastdev and https://mvnrepository.com/artifact/dev.feast) for Feast Java (since Feast Java is on 0.25.2 whereas the main repo is on 0.15) and update tutorials to reflect this.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Re-introducing Feast Java into main repo

feast-ci-bot · 2021-11-04T20:00:25Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

adchia · 2021-11-04T20:00:50Z

/test all

feast-ci-bot · 2021-11-04T20:00:54Z

@adchia: No jobs can be run with /test all.
The following commands are available to trigger jobs:

/test test-core-and-ingestion
/test test-serving
/test test-java-sdk
/test test-usage
/test test-golang-sdk

In response to this:

/test all

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.

adchia · 2021-11-04T20:01:30Z

/test test-core-and-ingestion

adchia · 2021-11-04T20:01:34Z

/test test-serving

adchia · 2021-11-04T20:01:44Z

/test test-java-sdk

codecov-commenter · 2021-11-04T20:07:50Z

Codecov Report

Merging #1997 (65c2ad7) into master (600d38e) will decrease coverage by 23.33%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1997       +/-   ##
===========================================
- Coverage   82.21%   58.88%   -23.34%     
===========================================
  Files         100      101        +1     
  Lines        8052     8106       +54     
===========================================
- Hits         6620     4773     -1847     
- Misses       1432     3333     +1901

Flag	Coverage Δ
integrationtests	`?`
unittests	`58.88% <ø> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../integration/online_store/test_universal_online.py	`13.90% <0.00%> (-85.57%)`	⬇️
.../integration/online_store/test_online_retrieval.py	`17.39% <0.00%> (-82.61%)`	⬇️
...fline_store/test_universal_historical_retrieval.py	`19.41% <0.00%> (-80.00%)`	⬇️
sdk/python/tests/utils/online_read_write_test.py	`20.68% <0.00%> (-79.32%)`	⬇️
...gration/registration/test_feature_service_apply.py	`31.25% <0.00%> (-68.75%)`	⬇️
sdk/python/feast/infra/online_stores/redis.py	`25.17% <0.00%> (-68.71%)`	⬇️
sdk/python/tests/data/data_creator.py	`34.78% <0.00%> (-65.22%)`	⬇️
...s/integration/registration/test_universal_types.py	`36.73% <0.00%> (-63.27%)`	⬇️
sdk/python/feast/infra/online_stores/datastore.py	`30.32% <0.00%> (-63.12%)`	⬇️
sdk/python/feast/infra/online_stores/dynamodb.py	`30.37% <0.00%> (-60.76%)`	⬇️
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 600d38e...65c2ad7. Read the comment docs.

adchia · 2021-11-04T20:12:38Z

/retest

adchia · 2021-11-04T20:13:11Z

/test test-serving

adchia · 2021-11-04T20:14:18Z

/test test-core-and-ingestion

adchia · 2021-11-04T20:14:27Z

/test test-java-sdk

feast-ci-bot · 2021-11-04T20:15:12Z

@adchia: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
test-core-and-ingestion	657fd1b687b98e6a19a6bfce1a4cb70e4067ff8c	link	`/test test-core-and-ingestion`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

adchia · 2021-11-04T20:24:10Z

/test test-java-sdk

adchia · 2021-11-04T21:03:15Z

/test test-serving

pyalex · 2021-11-05T11:19:33Z

.github/workflows/java_mirror.yml

@@ -0,0 +1,25 @@
+name: mirror


I think this part is obsolete. It used to mirror to internal repo in Gojek, but that repo is on 0.9-branch

pyalex · 2021-11-05T11:20:45Z

java/common-test/pom.xml

+            <version>5.6.2</version>
+        </dependency>
+        <dependency>
+            <groupId>org.springframework.kafka</groupId>


it seems like we bringing back too much

Ingestion part was supposed to be deleted from feast-java long time ago, but it's possible that not everything was deleted

As discussed, will in a followup PR remove obsolete sections

pyalex · 2021-11-05T11:27:00Z

Are we gonna support 0.9 registry? I'm confused by the goal of this resurrection. IMO, big part of it should die for good

woop · 2021-11-06T16:33:53Z

Are we gonna support 0.9 registry? I'm confused by the goal of this resurrection. IMO, big part of it should die for good

As an extension to this question and our conversation yesterday. Would it make sense to list out the exact functionality that will not be supported (and is slated for deletion) after this PR is merged?

Overall the PR looks good to me, as long as we agree a lot of the code will be deleted after merge. In terms of GitHub Actions: We should probably merge the different tests by category (master, pr, release, etc). I think it will be easier for us to manage.

Signed-off-by: Danny Chiao <[email protected]>

adchia · 2021-11-10T16:46:49Z

@pyalex @woop PTAL. Updated the description of the PR with TODOs. We can consolidate workflows in a followup PR

feast-ci-bot · 2021-11-11T04:30:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adchia, pyalex

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:

~~OWNERS~~ [adchia]

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

pyalex · 2021-11-11T04:30:59Z

/lgtm

adchia · 2021-11-12T01:31:38Z

@pyalex will fix some tests very soon

feast-ci-bot added do-not-merge/work-in-progress release-note labels Nov 4, 2021

feast-ci-bot added approved needs-kind size/XXL labels Nov 4, 2021

adchia marked this pull request as ready for review November 4, 2021 20:24

adchia requested review from achals, felixwang9817, tsotnet, woop and a team as code owners November 4, 2021 20:24

feast-ci-bot removed the do-not-merge/work-in-progress label Nov 4, 2021

github-actions bot assigned felixwang9817 Nov 4, 2021

adchia assigned pyalex and woop and unassigned felixwang9817 Nov 4, 2021

adchia added the kind/housekeeping label Nov 4, 2021

feast-ci-bot removed the needs-kind label Nov 4, 2021

pyalex reviewed Nov 5, 2021

View reviewed changes

adchia added 4 commits November 9, 2021 11:16

Moving Feast Java back into main repo under java/ package

731f12c

Signed-off-by: Danny Chiao <[email protected]>

Update infra scripts with new pom.xml structure

ed1a891

Signed-off-by: Danny Chiao <[email protected]>

Fix test-java-sdk.sh

29d0925

Signed-off-by: Danny Chiao <[email protected]>

Remove unneeded mirror

fa66cf5

Signed-off-by: Danny Chiao <[email protected]>

adchia force-pushed the stream_feature_view branch from 65c2ad7 to fa66cf5 Compare November 9, 2021 19:59

Update workflows with TODOs

52d8a59

Signed-off-by: Danny Chiao <[email protected]>

pyalex approved these changes Nov 11, 2021

View reviewed changes

feast-ci-bot added the lgtm label Nov 11, 2021

adchia merged commit 63680ba into feast-dev:master Nov 12, 2021

adchia deleted the stream_feature_view branch December 7, 2021 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving Feast Java back into main repo under java/ package #1997

Moving Feast Java back into main repo under java/ package #1997

adchia commented Nov 4, 2021 •

edited

Loading

feast-ci-bot commented Nov 4, 2021

adchia commented Nov 4, 2021

feast-ci-bot commented Nov 4, 2021

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

codecov-commenter commented Nov 4, 2021 •

edited

Loading

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

feast-ci-bot commented Nov 4, 2021 •

edited

Loading

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

pyalex Nov 5, 2021 •

edited

Loading

adchia Nov 9, 2021

pyalex Nov 5, 2021

pyalex Nov 5, 2021

adchia Nov 9, 2021

pyalex commented Nov 5, 2021 •

edited

Loading

woop commented Nov 6, 2021 •

edited

Loading

adchia commented Nov 10, 2021

feast-ci-bot commented Nov 11, 2021

pyalex commented Nov 11, 2021

adchia commented Nov 12, 2021

Moving Feast Java back into main repo under java/ package #1997

Moving Feast Java back into main repo under java/ package #1997

Conversation

adchia commented Nov 4, 2021 • edited Loading

feast-ci-bot commented Nov 4, 2021

adchia commented Nov 4, 2021

feast-ci-bot commented Nov 4, 2021

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

codecov-commenter commented Nov 4, 2021 • edited Loading

Codecov Report

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

feast-ci-bot commented Nov 4, 2021 • edited Loading

adchia commented Nov 4, 2021

adchia commented Nov 4, 2021

pyalex Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

adchia Nov 9, 2021

Choose a reason for hiding this comment

pyalex Nov 5, 2021

Choose a reason for hiding this comment

pyalex Nov 5, 2021

Choose a reason for hiding this comment

adchia Nov 9, 2021

Choose a reason for hiding this comment

pyalex commented Nov 5, 2021 • edited Loading

woop commented Nov 6, 2021 • edited Loading

adchia commented Nov 10, 2021

feast-ci-bot commented Nov 11, 2021

pyalex commented Nov 11, 2021

adchia commented Nov 12, 2021

adchia commented Nov 4, 2021 •

edited

Loading

codecov-commenter commented Nov 4, 2021 •

edited

Loading

feast-ci-bot commented Nov 4, 2021 •

edited

Loading

pyalex Nov 5, 2021 •

edited

Loading

pyalex commented Nov 5, 2021 •

edited

Loading

woop commented Nov 6, 2021 •

edited

Loading