-
Notifications
You must be signed in to change notification settings - Fork 143
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
Asyncio Listing and Inventory Report Integration #573
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi all, this is Bernard Han from Google GCS Team. I'm Hans' co-host this summer. Leaving some general comments on this public PR.
Also, are README changes in a separate commit or PR? |
Everything passes, so that's a good start :) I will start looking at this later today - there is quite a lot! Sorry for being slow getting here, got hit by covid. |
Oh no sorry to hear, covid sucks ;( |
I am thinking of adding a README change, and maybe a blog post on how to use the inventory report, once the PR is officially merged (in order to finalize all the details). |
Yes, we certainly need prose "howto" documentation around this, in our own docs source. I would recommend any blog focus on the process of development and speedup for your example case rather that a how-to-use piece. Of course, the two can link to one-another. I don't mind if the docs lag behind this PR, effectively making the new feature experimental. In fact, calling it an experimental feature isn't a bad idea, since we don't have a good feel of the set of use cases it might be pulled into, and there may be some that behave badly. |
Sounds good, I will write up a prose (internally) and a blog (shared externally) once all the changes are finalized for this PR. Meanwhile, should I add a couple comments on top of the inventory report logic, indicating this is currently experimental? |
Update: I fixed the typos that Bernard has pointed out in the latest commit @Magichan33. |
Yes, I think so |
Sounds good, just updated that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just small things left
👍 |
@martindurant
I've completed the integration of the inventory report service and asyncio listing logic based on our discussions.
The overview of the logic is as follows:
For listing, the user can optionally pass in a
inventory_report_info
argument as part of thekwargs
.If the argument is not passed in, then the listing logic is exactly the same as previously, thus it is backward compatible with existing implementation.
If the argument is passed in, the high-level is as follows:
inventory_report_info
has the correct fields included to fetch the inventory report.The listing performance is ~10x from my own experiments.
Latency for 1 - 6 should be trivial since:
In addition, if the user chooses to use the inventory report content directly for listing results, the speedup can be >100x.
Comments have been added throughout the implementation, and there are comprehensive test cases covering each method in the
InventoryReport
class.Would really appreciate it if any of you has any feedback, so that we can have this awesome feature integrated!