chore: migrate /ccu-info to new v1/user-info API#432
Conversation
colaboratory-team
left a comment
There was a problem hiding this comment.
Oh my, thanks for such a massive change for the migration. Hope it wasn't too bad. I can only say my review was cursory at best, and I depend on the passing tests. I believe this will be also soon used for integration tests (Kokoro?) and hit the real sandbox endpoints soon? Thanks again!
hosungs
left a comment
There was a problem hiding this comment.
Oops, reviewed/approved as colaboratory-team, may not matter, but just in case, approving as hosungs too. :) Thanks again!
|
@hosungs - Thanks for the quick review!
Actually, our E2E tests run as part of this PR pre-merge checks, and they hit real production endpoints I believe: So rest assured! |
amtoney524
left a comment
There was a problem hiding this comment.
lgtm - really nice refactor on top of the migration!! thanks Jack!
Why? Colab is in the process of deprecating old
/tun/m/*APIs. This PR migratesgetCcuInfo(also renamed togetConsumptionUserInfo) from/ccu-infoto newv1/user-infoAPI withget_ccu_consumption_info=true.Demo screencast showing GPU/TPU selector still working and new API calls logged with status 200.
There are 2 optimizations as part of this migration:
assignServerused to call bothgetSubscriptionTierandgetCcuInfoto retrieve server descriptors. After this migration, we can get everything we need to assign a server from the basicgetUserInfocall, reducing 2 HTTP calls to one.ConsumptionNotifierused to callgetSubscriptionTierwhile it already has access to aConsumptionUserInfoinstance via event emitter. After this migration, we can directly get subscription tier fromConsumptionUserInfowithout needing to callgetUserInfoagain, removing the redundant HTTP call.Internal tracking bug: b/485350944