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

[GCP-5388] Storage: Blob.exists() does not work within Batch context #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[GCP-5388] Storage: Blob.exists() does not work within Batch context #1

wants to merge 5 commits into from

Conversation

IlyaFaer
Copy link
Collaborator

@IlyaFaer IlyaFaer commented Jul 1, 2019 •
edited
Loading

IPR: 5388

Not very beautiful (that's how I see), but looks like it's the only easy solution. For now, exists() don't do requests while in batch context. It only adds request into list, which will be processed on exiting batch context. So exists() returns True after adding request into batch list, but it doesn't wait for requests to be processed. I've added object to pass it as target object into request (it will bind future request to this object) and to get result after exiting context

@IlyaFaer IlyaFaer requested a review from mf2199 July 1, 2019 12:43
@IlyaFaer IlyaFaer changed the title Storage: Blob.exists() does not work within Batch context Storage: Blob.exists() does not work within Batch context 5388 Jul 1, 2019
@IlyaFaer IlyaFaer requested review from mf2199 and removed request for mf2199 July 8, 2019 07:26
@IlyaFaer IlyaFaer added this to the 11 Jul 2019 milestone Jul 9, 2019
@IlyaFaer IlyaFaer removed this from the 11 Jul 2019 milestone Jul 25, 2019
@mf2199 mf2199 changed the title Storage: Blob.exists() does not work within Batch context 5388 [GCP-5388] Storage: Blob.exists() does not work within Batch context Jul 31, 2019
@mf2199 mf2199 requested review from emar-kar and sumit-ql August 2, 2019 21:25
@sumit-ql
Copy link

sumit-ql commented Aug 3, 2019

Overall, solution looks good, just couple of comments related to test coverage:

  1. Are we having a test where we are covering the case : "current.batch is not None".
  2. We should also have tests for FutureBoolResult class.

sumit-ql
sumit-ql reviewed Aug 3, 2019
View reviewed changes
storage/noxfile.py
)
Copy link

Choose a reason for hiding this comment

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

Is there any changes in nox file, if not, please remove this file from the the PR.

emar-kar
emar-kar approved these changes Aug 5, 2019
View reviewed changes
Copy link

@emar-kar emar-kar left a comment

Choose a reason for hiding this comment

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

LGTM.
Checked noxfile chgs and as I see there was a correction to pass black session.

@mf2199
Copy link
Collaborator

mf2199 commented Aug 5, 2019

LGTM.
Checked noxfile chgs and as I see there was a correction to pass black session.

@emar-kar You probably meant, blacken session.

@IlyaFaer
Copy link
Collaborator Author

IlyaFaer commented Aug 7, 2019

@mf2199, @emar-kar, @sumit-ql, I think we have some problems with our master, 'cause in PPR I don't have changes in noxfile. We have bash-script for master sync, use it (watch for paths and repo names - yours can be different)

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

@emar-kar emar-kar emar-kar approved these changes

@sumit-ql sumit-ql sumit-ql approved these changes

@mf2199 mf2199 mf2199 approved these changes

Assignees
No one assigned
Labels
api: storage type: feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IlyaFaer @sumit-ql @mf2199 @emar-kar