Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ingestion should fail immediately when there are no valid stores #12

Closed
zhilingc opened this issue Dec 21, 2018 · 6 comments
Closed

Ingestion should fail immediately when there are no valid stores #12

zhilingc opened this issue Dec 21, 2018 · 6 comments
Assignees

Comments

@zhilingc
Copy link
Collaborator

zhilingc commented Dec 21, 2018

Expected Behavior

Starting a job with invalid stores (e.g. using redis as a warehouse store) should not be allowed, and should fail quickly - ideally at the graph building step of ingestion.

Current Behavior

Starting a job with invalid stores will successfully send the job to the runner, which will run to completion (or indefinitely, in the case of streaming jobs). The errors will be logged, but the pipeline will run with no problems.

Steps to reproduce

  • Register a feature with its warehouse sink pointing to a serving store (e.g. redis)
  • Run a job (direct runner is the best way to view errors)
  • Pipeline runs successfully, a successful response is returned to the caller

Possible Solution

PR #11 is a band-aid solution to this problem: it checks the store types at registration, ensuring that a feature is unable to use a serving store for warehousing, but ideally ingestion error out properly during graph building.

@tims
Copy link
Contributor

tims commented Dec 24, 2018

@zhilingc I think you misunderstood the code it's not a "bandaid" :)

It does fail at graph build time if the feature references a store that is not found in the specs service.
See Specs.validate(). Am I missing something?

@tims
Copy link
Contributor

tims commented Dec 24, 2018

Sorry I misread this issue.

The fix is in PR #15.

@zhilingc
Copy link
Collaborator Author

It's a band-aid because if someone alters the DB the job will fail since the checks are only at feature registration time.

And nice! If it works on remote flink we're golden :)

@tims
Copy link
Contributor

tims commented Dec 31, 2018

This should stop those self imposed DoS events every 10 minutes too.

@zhilingc zhilingc mentioned this issue Dec 31, 2018
@tims
Copy link
Contributor

tims commented Dec 31, 2018

We can close this now?

@zhilingc
Copy link
Collaborator Author

zhilingc commented Jan 3, 2019

Yep. Thanks for the hard work :)

@zhilingc zhilingc closed this as completed Jan 3, 2019
Yanson pushed a commit to Yanson/feast that referenced this issue Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants