Skip to content

fix(bigquery-firestore-export): notify this instance's topic when linking a config - #2960

Open
IzaakGough wants to merge 3 commits into
kitsfrom
fix/bfe-link-notification-topic
Open

fix(bigquery-firestore-export): notify this instance's topic when linking a config#2960
IzaakGough wants to merge 3 commits into
kitsfrom
fix/bfe-link-notification-topic

Conversation

@IzaakGough

@IzaakGough IzaakGough commented Aug 19, 2026

Copy link
Copy Markdown

Setting TRANSFER_CONFIG_NAME to 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, while processMessages listens on kit-<instanceId>-processMessages, so run output never reached Firestore and nothing logged an error.

The link path now sends an update masked to notification_pubsub_topic alone, 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 .env values, 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 plus latest. A redeploy changed nothing, and an instance without TRANSFER_CONFIG_NAME still created and reused its own query. 21 unit tests pass.

…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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@IzaakGough
IzaakGough marked this pull request as ready for review August 20, 2026 09:38
@cabljac

cabljac commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

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:

  1. A data team already has a scheduled query, notifying topic analytics-run-events. They have their own subscriber on it doing something unrelated to us, say alerting on failed runs.
  2. Someone else in the org installs the kit and sets TRANSFER_CONFIG_NAME to that query, because reusing an existing query is exactly what the setting is for.
  3. On deploy we set the topic to kit-<instanceId>-processMessages. Our side now works.
  4. Their subscriber goes silent. Not an error, just nothing arriving. Their code didn't change, their query didn't change, the run history in the console looks fine. The only visible difference is one field on the config, and nothing logged that we changed it.
  5. Later the kit gets removed. The config is still pointing at our topic, which nobody consumes now. Nothing restores analytics-run-events, because we never wrote it down.

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 <name>_{run_time|"%H%M%S"}, config in this project, config in bigqueryDatasetLocation. helper.ts:218 hardcodes that template and helper.ts:149 builds the query from config.projectId, so a differently shaped config surfaces as a BigQuery table-not-found rather than anything pointing at the mismatch, and processMessages has retry: true (index.ts:114) so it repeats instead of failing once. Documenting that shape as the only supported one is a fine answer, we just shouldn't leave it implied.

@cabljac

cabljac commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants