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.

Sign up for GitHub

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

Jump to bottom

fix: Support param timeout when persisting #3504

Closed
wants to merge 6 commits into from
Closed

fix: Support param timeout when persisting #3504

wants to merge 6 commits into from

Conversation

sudohainguyen
Copy link
Collaborator

What this PR does / why we need it: to support declaring timeout when persisting historical retrieval job

Which issue(s) this PR fixes:

Fixes #

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found 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

@sudohainguyen sudohainguyen changed the title WIP: fix: Support param timeout when persisting fix: Support param timeout when persisting Feb 21, 2023
@gbmarc1
Copy link
Contributor

gbmarc1 commented Feb 22, 2023

Hello @sudohainguyen !
I am working on a similar PR but for to_df and to_arrow methods.

  • I think it would be good to add unit-tests to those methods.
  • What about Optional[int] with default None if you don't want a timeout.

@sudohainguyen
Copy link
Collaborator Author

Hello @sudohainguyen ! I am working on a similar PR but for to_df and to_arrow methods.

  • I think it would be good to add unit-tests to those methods.

sure let me see

@sudohainguyen
Copy link
Collaborator Author

sudohainguyen commented Feb 23, 2023 •
edited
Loading

  • What about Optional[int] with default None if you don't want a timeout.

@gbmarc1 there should always be a timeout I believe

@sudohainguyen
fix: Support param timeout when persisting
f416337 
Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>
@sudohainguyen
chore: add type hint to persist methods
d48a089 
Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>
@sudohainguyen
chore: add type hint to to_bigquery method
cd26b6f 
Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>
@sudohainguyen
fix: set default timeout to None in base offline store class
65a94dd 
Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>
@sudohainguyen
Copy link
Collaborator Author

just rebased

@sudohainguyen
fix: set default timeout to None in to_bigquery
69a8091 
Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>
@sudohainguyen
chore: update test case due to change of param
56e790e 
Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>
@sudohainguyen
Copy link
Collaborator Author

@achals do you think we still need this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achals achals achals left review comments

@niklasvm niklasvm Awaiting requested review from niklasvm

@kevjumba kevjumba Awaiting requested review from kevjumba

@yelo-blood yelo-blood Awaiting requested review from yelo-blood

@sfc-gh-madkins sfc-gh-madkins Awaiting requested review from sfc-gh-madkins

@chhabrakadabra chhabrakadabra Awaiting requested review from chhabrakadabra

@mavysavydav mavysavydav Awaiting requested review from mavysavydav

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sudohainguyen @feast-ci-bot @gbmarc1 @achals