Change default resource locations for functions#10414
Change default resource locations for functions#10414
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements dynamic region resolution for Cloud Functions, moving away from the hardcoded us-central1 default. It introduces logic to determine the optimal region based on trigger types and the location of associated resources like Firestore databases, Storage buckets, and Realtime Database instances. Feedback focuses on performance optimizations, specifically recommending parallelizing asynchronous resource lookups using Promise.all and implementing caching for Storage and Realtime Database metadata to avoid redundant API calls. Additionally, it was suggested to map multi-regional locations like nam5 to us-east1 to better distribute the load.
This creates a local cache of Firestore buckets and RTDB instances during deployment to limit API calls.
a988938 to
2505bca
Compare
| const eventType = eventTrigger.eventType; | ||
|
|
||
| if ( | ||
| eventType.startsWith("google.cloud.pubsub.") || |
There was a problem hiding this comment.
I think we talked yesterday about maybe this code could be simplified to rather than checking event type just check if the target region is global and if so target us-east1. That could be robust to new global things not accidentally defaulting to us-central1
Do you think that would work? Better or not?
| const db = await getDatabase(endpoint.project, databaseId); | ||
| const locationId = db.locationId.toLowerCase(); | ||
|
|
||
| if (locationId === "nam5" || locationId === "nam7") return "us-central1"; |
There was a problem hiding this comment.
We have location resolution logic throughout all this code. It does match the design. But, taking a step back... I think we are always suggesting the a single mapping from (multi-region) to region. It just so happens that a bunch of regions don't matter for each event type.
Do you think it'd be clearer to have something like
getFunctionRegionFromEventSourceRegion() {
// global case
// multi-region cases (nam5, etc.)
// multi-regional cases (us, eu, asia)
// single region default (return the region)
}
Then it's basically "If you're in scope to go near the default we call this one function to do the mapping" and if you're not in scope to move you fall through to the old default of us-central1.
This would only break down if we were resolved differently for different event types, e.g., Firestore triggers in nam5 resolve to a different region than Cloud Storage. I have no idea if that would ever happen?
I think the flip side argument to this is that some of these regions actually only really make sense for specific services and we're sort of conflating all that.
I'm not 100% sold on my suggestion so curious your thoughts.
| return "us-east1"; | ||
| } | ||
|
|
||
| if (isGlobalAILogicEndpoint(endpoint)) { |
There was a problem hiding this comment.
Should we be doing anything for non-global AI logic endpoints?
The functions emulator should recognize if the region is REGION_TBD, and set the default as us-central1 in that case.
This updates the default resource location for various functions from us-central1. This should only impact NEW functions, not the location of existing functions. The exhaustive list includes: