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

Remove historical feature in serving store #87

Merged
merged 8 commits into from
Feb 26, 2019

Conversation

pradithya
Copy link
Collaborator

Remove historical value of feature in serving store. This is incomplete solution and currently is blocked by the issue with late data overwriting newer data.

It contains breaking changes in serving and all the client will have to be updated

/hold

@tims
Copy link
Contributor

tims commented Jan 18, 2019

/lgtm

@tims
Copy link
Contributor

tims commented Jan 18, 2019

/lgtm cancel

@feast-ci-bot feast-ci-bot removed the lgtm label Jan 18, 2019
@pradithya pradithya mentioned this pull request Jan 21, 2019
@pradithya pradithya force-pushed the no_historical_feature branch from dcf244f to fc28b19 Compare January 21, 2019 08:52
@pradithya pradithya changed the title No historical feature No historical feature in serving store Jan 21, 2019
@pradithya pradithya changed the title No historical feature in serving store Remove historical feature in serving store Feb 12, 2019
@pradithya pradithya force-pushed the no_historical_feature branch from ce63292 to 9306d02 Compare February 12, 2019 07:10
@pradithya
Copy link
Collaborator Author

/hold cancel

@pradithya
Copy link
Collaborator Author

It's ready for review, let me know if you are against the new API structure
/assign @tims
/assign @zhilingc

@tims
Copy link
Contributor

tims commented Feb 14, 2019

/retest

Copy link
Contributor

@tims tims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy with the new API, but I think we should remove timestamp range filter completely. From what I can tell it's used to filter our the returned values server side. I think we should leave that up to the user.

Other notes inline.

@pradithya
Copy link
Collaborator Author

@tims I updated the PR to exclude server-side timestamp filtering. The timestamp filter is now done in client side.

@tims
Copy link
Contributor

tims commented Feb 20, 2019

Last feedback, ts_range in the sdk should be DateTime objects not strings.

@pradithya
Copy link
Collaborator Author

/retest

@tims
Copy link
Contributor

tims commented Feb 25, 2019

/approve

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tims

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:

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

@pradithya
Copy link
Collaborator Author

@zhilingc can you please review

@pradithya pradithya force-pushed the no_historical_feature branch from a6d66cf to 317fdd3 Compare February 25, 2019 09:39
@zhilingc
Copy link
Collaborator

/lgtm

@feast-ci-bot feast-ci-bot merged commit 2718eda into feast-dev:master Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants