Introduce datatypes/java module for proto generation

[ches]

Rather than the Maven protobuf plugin running on the same symlinked definitions in several Java modules, localize this process into one module that the others depend on.

This provides a single module that can be depended on by third-party extensions with the bare minimum of dependencies.

Also removes proto files that are no longer used.

The Maven protobuf plugin includes source .proto files when packaging by default, so third parties interested in consuming them (like us, for Scala SDK code generation) could potentially get them from the Feast Java SDK artifact at build time, without this PR. However, it's possible the SDK may add dependencies over time (auth libraries come to mind) that not all extenders will want.

From a Feast developer perspective also, it seems cleaner to build the mostly ubiquitous generated code once, rather than have redundant build products under the target/ directories of several modules and introducing the worry that those caches might become inconsistent.

I've named the module datatypes from a convention I've used for situations like this in the past, but perhaps that's not suggestive of gRPC service code also generated therein. I'm open to naming suggestions, or if we'd like to put pure data vs. service code in separate modules, I'm happy to make that change too. It just seemed like overkill to me for now (we need both for a Scala SDK anyway).

I will also backport this to v0.3-branch if we have consensus.

[feast-ci-bot]

Hi @ches. 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.

[ches]

@davidheryanto Maybe you can help us here if you already have Maven configuration for publishing the SDK to Sonatype. Things can be published later by a credential holder, but we can include any needed build config in this branch so it gets cherry-picked for backport. (Not sure if Sonatype will approve a group ID of feast, let's see).

I think CI configuration for publishing on release builds can be a separate PR, including the SDK too, unless you disagree. You probably know best how you'd like that automation to work, how secrets are handled in Prow, etc.

[davidheryanto]

Hi @ches
Yes, we already have a PR for the publishing of Feast library to Sonatype (which will sync it to Maven Central) #394 The group id we use is dev.feast
Will include the publishing of these protos as well in another PR.

[woop]

/ok-to-test

[ches]

/retest

[ches]

The test failures are maybe flaky, they're timeouts and I don't expect any functional change from this PR, but I don't have much ability to troubleshoot beyond the raw build log in Prow so if you think/see otherwise please let me know.

[woop]

@davidheryanto @khorshuheng can either of you have a look at these tests? It doesnt seem like we are solving the root cause of these flaky tests. They keep randomly failing.

[khorshuheng]

I will take a look tonight.

[davidheryanto]

The failing test currently is because we hardcode the revision number in the test script. Will make a quick update.

[davidheryanto]

After this PR is merged to master, I think test-end-to-end batch should succeed
#395

[davidheryanto]

/retest

[ches]

Happy New Year!

CI looks green now, is there anything more I should do here?

[woop]

After this PR is merged to master, I think test-end-to-end batch should succeed
#395

@davidheryanto the above PR is now merged.

@ches Is this PR consistent with #394? Some comments

• I think it makes sense to have this as a separate module. I am fine with the name. I would prefer to not add more folders to the top level of the repo, but that would create an inconsistency with how we structure submodules so perhaps this is fine for the time being.
• Porting to 0.3 makes sense.
• Thanks for removing unused protos.

@khorshuheng @davidheryanto are we happy with approving this PR? You both have more Java fu than I do :)

[ches]

Rebased, which entailed updating to dev.feast group ID following #394.

[davidheryanto]

/lgtm

[ches]

Resolved conflicts after #406 (changed back to ${revision} for Feast version in POMs)

[davidheryanto]

/lgtm

[woop]

/approve

[feast-ci-bot]

[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

Needs approval from an approver in each of these files:

• OWNERS [woop]

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

[ches]

/retest

[ches]

Hmm, does Prow build the magic GitHub merge ref for speculatively merging the PR to master? This latest failure comment from the bot says commit 1cf9e4c, which is the one from my last rebase that passed yesterday. Trying to understand why it built again (and failed).

The current rerun is extremely slow, took more than 20 minutes to reach the point of pytest running, 10 more now with no test output… I wonder if the host executing it is unhealthy. (Edit: failures are now from TimeoutErrors).

[woop]

Hmm, does Prow build the magic GitHub merge ref for speculatively merging the PR to master? This latest failure comment from the bot says commit 1cf9e4c, which is the one from my last rebase that passed yesterday. Trying to understand why it built again (and failed).

The current rerun is extremely slow, took more than 20 minutes to reach the point of pytest running, 10 more now with no test output… I wonder if the host executing it is unhealthy. (Edit: failures are now from TimeoutErrors).

@davidheryanto can you reply on @ches's first question.

@ches currently tests are not based on readiness but instead assume that the system will be ready at some point in time and will start the tests which then time out. Ideally we would stage the system, wait until it is ready (up to some point in time), and then start the tests when everything is ready.

We're investigating the logs of the Kubernetes cluster right now to see if we can figure out why this is so flaky and slow. Nothing jumps out yet. Unfortunately we don't have the bandwidth to allocate somebody to redo this setup right now, but it's one of the biggest things on my radar to resolve.

[davidheryanto]

@ches Yes, before running tests on pull request, Prow will merge the latest commit on pull request with the base branch, to make sure that the test runs succesfully after the pull request is merged in.

[ches]

Thanks guys for the answers.

The latest failure is same as the one on #407 that seemed to be fixed by #412, but the latter is on master now so maybe the wait time is sometimes still not enough? Or it’s exacerbated by cluster conditions. It’d be good to eventually get a retry strategy in place for integration tests instead of relying on plain sleeps, not sure if pytest or extra plugins for it may have some integration utilities like that already.

Are we confident this PR is not a source of issues and can move forward? This is the base in a chain of rebases for a few things I’m working on.

[woop]

/test test-end-to-end-batch