FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Farfetch fork from 0.7 to support databricks integration by rafalzydowicz · Pull Request #1076 · feast-dev/feast · GitHub

Repository navigation

Farfetch fork from 0.7 to support databricks integration - #1076

Merged
woop merged 11 commits into
feast-dev:v0.7-branchfrom
rafalzydowicz:v0.7-branch-spark
Feb 7, 2021
Merged

woop merged 11 commits into
feast-dev:v0.7-branchfrom
rafalzydowicz:v0.7-branch-spark

Conversation

Copy link
Copy Markdown

What this PR does / why we need it:

This PR adds support for databricks integration. On the server side, this means a new DatabricksJobManager. There have been minor changes to some common classes and extraction of some others to common modules to avoid unwanted beam dependencies leaking into our Spark jars. Otherwise, most changes are contained under spark and databricks folders. On the python client side, we have added support for azure data lake storage for the staging location. It requires a AZURE_CLIENT_ID, AZURE_TENANT_ID, and AZURE_CLIENT_SECRET. We are in the process of implementing device code flows for a few future release.

There is also a databricks emulator, or facade really, which provides a databricks API layer and runs Spark locally. It's not currently being used in our testing.

Ingestion metric naming conventions along with tags have been retained, with slightly altered prefixes on account of Spark source names and executor identifiers. We continued to use the original StatsdReporter and massaged the naming to allow tags to be included. Only Inflight and Deadletter metrics are currently implemented.

We are aware that v0.8 has a new Spark implementation, which is why we are merging this against v0.7.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

No API changes

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rafalzydowicz
To complete the pull request process, please assign woop
You can assign the PR to them by writing /assign @woop in a comment when ready.

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

Details 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

Copy link
Copy Markdown
Collaborator

Hi @rafalzydowicz. Thanks for your PR.

I'm waiting for a feast-dev 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.

Details

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.

woop commented Oct 30, 2020

Copy link
Copy Markdown
Member

@rafalzydowicz are you still actively working on this rebase?

Copy link
Copy Markdown
Author

@rafalzydowicz are you still actively working on this rebase?

Yes, we are developing against it on our fork, but this PR is ready. The integration tests are failing, but they are failing on the v7 branch in general (unless I'm missing something) and the jupyter build push because the image isn't found. Would you want these all fixed on this branch?

As an aside, considering your active development of spark on master, what is our goal with this PR?

woop commented Nov 1, 2020

Copy link
Copy Markdown
Member

@rafalzydowicz are you still actively working on this rebase?

Yes, we are developing against it on our fork, but this PR is ready. The integration tests are failing, but they are failing on the v7 branch in general (unless I'm missing something) and the jupyter build push because the image isn't found. Would you want these all fixed on this branch?

Ok, well we will need to fix the tests then. Will keep it open for the time being.

As an aside, considering your active development of spark on master, what is our goal with this PR?

Two reasons to get this merged in.

  1. It will take a while for 0.7 users to move to the 0.8+ architecture since its fundamentally different, so it makes sense to keep developing on 0.7 for now.
  2. The databricks specific functionality that you have here, especially around Delta and SparkSQL, will be useful for the development we will do in 0.9 (when we try and add Delta support). At the start that will be open source only, but we will probably add databricks API support later. You've already done most of the heavy lifting to add that support, even if we dont use the code verbatim.

Copy link
Copy Markdown
Author

@woop, docker build is still failing because it can't find images on docker.io. This may be related to a bug with buildkit mentioned at the bottom of https://docs.docker.com/develop/develop-images/build_enhancements/. Can you please have a look?

woop commented Nov 4, 2020

Copy link
Copy Markdown
Member

@woop, docker build is still failing because it can't find images on docker.io. This may be related to a bug with buildkit mentioned at the bottom of https://docs.docker.com/develop/develop-images/build_enhancements/. Can you please have a look?

Thanks @rafalzydowicz. Will take a look as soon as we get through the 0.8 release.

Copy link
Copy Markdown
Collaborator

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

Test name Commit Details Rerun command
test-end-to-end-gcp 87b4218 link /test test-end-to-end-gcp
python-sdk-integration-test 87b4218 link /test python-sdk-integration-test
test-end-to-end-aws 87b4218 link /test test-end-to-end-aws
test-end-to-end-sparkop 87b4218 link /test test-end-to-end-sparkop
test-end-to-end-azure 87b4218 link /test test-end-to-end-azure
test-telemetry 87b4218 link /test test-telemetry

Full PR test history. Your PR dashboard.

Details

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.

woop added the kind/feature New feature or request label Feb 7, 2021
woop merged commit 5b426a7 into feast-dev:v0.7-branch Feb 7, 2021
woop mentioned this pull request Feb 8, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL