fix(bigquery-firestore-export): notify this instance's topic when linking a config - #2960
fix(bigquery-firestore-export): notify this instance's topic when linking a config#2960IzaakGough wants to merge 3 commits into
Conversation
…king a config A linked config kept publishing completion notifications to whatever topic it already had, so processMessages never ran and no run output reached Firestore. The link path now sends an update masked to notification_pubsub_topic only, and skips it when the topic already matches.
There was a problem hiding this comment.
Code Review
This pull request updates the BigQuery Firestore Export kit to automatically point the completion notifications of a linked scheduled-query config to the current instance's Pub/Sub topic. It introduces the updateNotificationTopic helper function to perform this update, ensuring other configuration settings remain unchanged, and updates the documentation, logging, and unit tests accordingly. I have no feedback to provide as the implementation is clean and well-tested.
|
The mask scoping is exactly right, keeping the update to notification_pubsub_topic alone is the whole point. Reusing the full update path would have rewritten params.query at dts.ts:150, which is the opposite of adopting someone else's config. What I want spelled out in the README is that repointing the topic is a takeover, and a one way one. notification_pubsub_topic is a single string on the proto (transfer.proto:270), so a config has exactly one place to notify and we are now claiming it. Concretely, the case that worries me:
Step 4 is the expensive one. A silent stop with no error is much harder to track down than a loud failure. Same shape if two kit instances adopt the same config: last deploy wins, the earlier instance goes quiet, which is the failure this PR is fixing one level up. So two asks. First, log the previous topic at warn in updateNotificationTopic before overwriting it, so step 5 is recoverable. When we take over something we don't own, a breadcrumb to undo it costs us one line. Second, the README currently says the kit "changes nothing else", which is true of the update mask but reads as though linking is non-invasive. Please make the takeover explicit there, with the point that the config's previous notification target stops receiving runs. While you're in the README, add what adoption assumes: destination table template of |
|
Worth saying that this bug is inherited rather than something the port introduced. The original extension does the same thing at bigquery-firestore-export/functions/src/index.ts:42-77 in GoogleCloudPlatform/firebase-extensions: fetch the named config, write it to Firestore, never touch the notification topic. Its processMessages is wired to ext-${EXT_INSTANCE_ID}-processMessages (extension.yaml:70), so anyone using TRANSFER_CONFIG_NAME there gets the same silent no-op, and from what I can see there's no issue filed for it. Can you open one on that repo with the repro you did here and link it back to this PR? The doc gap needs the same treatment, PREINSTALL.md there rather than the README since the README's Details section is generated from it. |
Repointing an adopted config's notifications is a takeover: a config notifies one topic, so whatever consumed the previous one goes quiet with no error and nothing recording what to restore. Log the previous topic at warn before overwriting it, and say so in the README along with the shape linking assumes.
Setting
TRANSFER_CONFIG_NAMEto adopt an existing scheduled query read the config and stored it, but never touched its notification topic. The adopted query kept notifying whatever topic it already had, whileprocessMessageslistens onkit-<instanceId>-processMessages, so run output never reached Firestore and nothing logged an error.The link path now sends an update masked to
notification_pubsub_topicalone, and skips the call when the topic already matches. That scoping is deliberate: reusing the full update path would overwrite the adopted query, schedule, table template and dataset with this instance's.envvalues, which is the opposite of adopting someone else's query.Verified on a real project. On current code, an adopted query notified its old topic, never invoked
processMessages, and wrote no run documents. With the fix the same config notifies the kit's topic, its query, schedule, table template and dataset are unchanged, and a run wrote three output rows pluslatest. A redeploy changed nothing, and an instance withoutTRANSFER_CONFIG_NAMEstill created and reused its own query. 21 unit tests pass.