Skip to content

Upgrade ingestion to allow for in-flight updates to feature sets for sinks - #757

Merged
feast-ci-bot merged 2 commits into
feast-dev:masterfrom
pyalex:dynamic-sink-api
Jun 9, 2020
Merged

Upgrade ingestion to allow for in-flight updates to feature sets for sinks#757
feast-ci-bot merged 2 commits into
feast-dev:masterfrom
pyalex:dynamic-sink-api

Conversation

@pyalex

@pyalex pyalex commented May 29, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What this PR does / why we need it:

There are several improvements:

  • Sink API accepts PCollection<FeatureSetSpec> and both BQ and redis can change their processing according to new specs in-flight
  • BQ sink writes through file loads (to gs) - that enables schema update & also might be less expensive
  • BQ sink produces sucessful inserts stream - enables metrics on this sink
  • Dataflow JobManager stop+start job when specs is updated. This is caused by the bug in Dataflow that is losing state of PCollectionView on update.
  • Kafka Reader in ingestion now has stable group id (across restarting jobs)

Which issue(s) this PR fixes:

Unblock #761

Does this PR introduce a user-facing change?:

@pyalex pyalex changed the title Dynamic FeatureSet schemas in Sink Dynamic (from PCollection) FeatureSet schemas in Sink May 29, 2020
@pyalex

pyalex commented May 29, 2020

Copy link
Copy Markdown
Collaborator Author

/hold

@pyalex pyalex changed the title Dynamic (from PCollection) FeatureSet schemas in Sink Dynamic FeatureSet schemas in Sink May 29, 2020
@pyalex pyalex changed the title Dynamic FeatureSet schemas in Sink Dynamic FeatureSet specs in Sink May 29, 2020
@pyalex pyalex changed the title Dynamic FeatureSet specs in Sink [Ingestion Job] Dynamic FeatureSet specs in Sink May 29, 2020
@pyalex
pyalex force-pushed the dynamic-sink-api branch 5 times, most recently from 48a3f19 to 2ae9135 Compare June 5, 2020 08:10
@pyalex

pyalex commented Jun 5, 2020

Copy link
Copy Markdown
Collaborator Author

/test test-end-to-end-batch-dataflow

@pyalex pyalex changed the title [Ingestion Job] Dynamic FeatureSet specs in Sink [Ingestion Job] In-Flight update of FeatureSetSpecs in Sinks (BQ & redis). BQ table schema update as part of data load Jun 5, 2020
@pyalex

pyalex commented Jun 5, 2020

Copy link
Copy Markdown
Collaborator Author

/unhold

@pyalex pyalex added the kind/feature New feature or request label Jun 5, 2020
@pyalex

pyalex commented Jun 8, 2020

Copy link
Copy Markdown
Collaborator Author

I've left some comments, but they aren't fundamental changes. Just small nitpicks. Overall the PR looks good to me. One thing that I would ask for is improved JavaDoc coverage. This part of the code base can be hard to parse, so comments can only help the next person.

Added some details on big query writing flow as well

woop
woop reviewed Jun 8, 2020
View reviewed changes
Comment thread core/src/main/resources/application.yml Outdated
woop
woop reviewed Jun 8, 2020
View reviewed changes
Comment thread docs/roadmap.md Outdated
@pyalex
pyalex force-pushed the dynamic-sink-api branch from 7e70e5b to 62a6dc3 Compare June 8, 2020 11:11
@pyalex
pyalex force-pushed the dynamic-sink-api branch from c6e710f to 50e8ccc Compare June 9, 2020 05:04
@woop woop changed the title [Ingestion Job] In-Flight update of FeatureSetSpecs in Sinks (BQ & redis). BQ table schema update as part of data load Upgrade ingestion to allow for in-flight updates to feature sets for sinks Jun 9, 2020
@woop

woop commented Jun 9, 2020

Copy link
Copy Markdown
Member

/lgtm

@feast-ci-bot

feast-ci-bot commented Jun 9, 2020

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described 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

@feast-ci-bot
feast-ci-bot merged commit 577edca into feast-dev:master Jun 9, 2020
@feast-ci-bot

feast-ci-bot commented Jun 9, 2020

Copy link
Copy Markdown
Collaborator

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

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

In response to this:

What this PR does / why we need it:

There are several improvements:

  • Sink API accepts PCollection<FeatureSetSpec> and both BQ and redis can change their processing according to new specs in-flight
  • BQ sink writes through file loads (to gs) - that enables schema update & also might be less expensive
  • BQ sink produces sucessful inserts stream - enables metrics on this sink
  • Dataflow JobManager stop+start job when specs is updated. This is caused by the bug in Dataflow that is losing state of PCollectionView on update.
  • Kafka Reader in ingestion now has stable group id (across restarting jobs)

Which issue(s) this PR fixes:

Unblock #761

Does this PR introduce a user-facing change?:

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

ches commented Jun 16, 2020

Copy link
Copy Markdown
Member
  • Dataflow JobManager stop+start job when specs is updated. This is caused by the bug in Dataflow that is losing state of PCollectionView on update.

Is there some further reference to what this about, that can be included in the description?

@ches

ches commented Jun 16, 2020

Copy link
Copy Markdown
Member
  • Kafka Reader in ingestion now has stable group id (across restarting jobs)

Does this obviate #760 and close #646? It'd be ideal if this sort of change could be unbundled from a big PR like this—easier to track the status/closure of issues, easier to backport fixes versus enhancements, etc.

@woop

woop commented Jun 19, 2020

Copy link
Copy Markdown
Member
  • Kafka Reader in ingestion now has stable group id (across restarting jobs)

Does this obviate #760 and close #646? It'd be ideal if this sort of change could be unbundled from a big PR like this—easier to track the status/closure of issues, easier to backport fixes versus enhancements, etc.

I believe it does close those, will let @pyalex confirm. Yea, ideally those changes would not be bundled into one PR.

@pyalex

pyalex commented Jun 19, 2020

Copy link
Copy Markdown
Collaborator Author

@ches @woop Correct, issue with kafka consumer group should be solved and it was rather side effect not a main purpose of this PR

@ches

ches commented Jun 19, 2020

Copy link
Copy Markdown
Member

it was rather side effect not a main purpose of this PR

Understandable, glad for a fix getting mainlined either way!

This was referenced Jun 19, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@woop woop woop approved these changes
@davidheryanto davidheryanto Awaiting requested review from davidheryanto
@khorshuheng khorshuheng Awaiting requested review from khorshuheng
@zhilingc zhilingc Awaiting requested review from zhilingc
+1 more reviewer
@Yanson Yanson Yanson left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@woop woop

Labels

approved kind/feature New feature or request lgtm size/XXL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pyalex @woop @feast-ci-bot @ches @Yanson