Skip to content

Async job management#361

Merged
feast-ci-bot merged 12 commits intofeast-dev:masterfrom
zhilingc:async
Dec 17, 2019
Merged

Async job management#361
feast-ci-bot merged 12 commits intofeast-dev:masterfrom
zhilingc:async

Conversation

@zhilingc
Copy link
Copy Markdown
Collaborator

@zhilingc zhilingc commented Dec 12, 2019

Currently jobs are kicked off in a synchronous way when feature sets are applied. This is an issue because it is:

  • prone to race conditions which leads to failures when multiple feature sets are being applied.
  • unresponsive for the user and inefficient on feast's side, particularly when the dataflow runner is used

This PR moves all job management to a worker thread.

The goal is to:

  • Allow feature sets to be applied without blocking for a more snappy user experience
  • Allow job updates to happen in the background without the user knowing about it
  • Test and prevent any race conditions

@feast-ci-bot
Copy link
Copy Markdown
Collaborator

feast-ci-bot commented Dec 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhilingc

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

@zhilingc
Copy link
Copy Markdown
Collaborator Author

zhilingc commented Dec 12, 2019

/hold

@woop
Copy link
Copy Markdown
Member

woop commented Dec 12, 2019

This is still a [WIP]?

@zhilingc
Copy link
Copy Markdown
Collaborator Author

zhilingc commented Dec 12, 2019

Yeah. I haven't implemented safe ingestion yet (hence the e2e tests are failing)

@ches ches mentioned this pull request Dec 13, 2019
Closed
@zhilingc
Copy link
Copy Markdown
Collaborator Author

zhilingc commented Dec 16, 2019 •
edited
Loading

Pushed changes for safe ingestion: This mainly involves

  • maintaining feature set status via the list of jobs that have to run,
  • returning this status to clients via the GetFeatureSet/ListFeatureSet methods.
  • Clients poll until feast indicates that the it is safe to ingest the feature set, then ingest the data.

Unfortunately this might introduce quite a bit of lag, particularly if a user submits their feature set directly after feast starts updating its jobs. Will need to think this through and fix in a further PR, but it works decently in the interim.

The change to the protos is more or less consistent with what is described in the issue #363.

@zhilingc
Copy link
Copy Markdown
Collaborator Author

zhilingc commented Dec 16, 2019

/hold cancel

woop
woop requested changes Dec 17, 2019
View reviewed changes
@zhilingc
Copy link
Copy Markdown
Collaborator Author

zhilingc commented Dec 17, 2019

/retest

woop
woop requested changes Dec 17, 2019
View reviewed changes
@zhilingc
Copy link
Copy Markdown
Collaborator Author

zhilingc commented Dec 17, 2019

/retry

@woop
Copy link
Copy Markdown
Member

woop commented Dec 17, 2019

/lgtm

@feast-ci-bot feast-ci-bot merged commit f2d4db5 into feast-dev:master Dec 17, 2019
@woop woop mentioned this pull request Dec 17, 2019
Merged
@ches ches mentioned this pull request Jan 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@woop woop woop requested changes

@pradithya pradithya Awaiting requested review from pradithya

@tims tims Awaiting requested review from tims

@davidheryanto davidheryanto Awaiting requested review from davidheryanto

@thirteen37 thirteen37 Awaiting requested review from thirteen37

Assignees

@woop woop

Labels

approved lgtm size/XXL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zhilingc @feast-ci-bot @woop