Skip to content

feat(firestore-bigquery-export): add the sync queue params - #3129

Merged
cabljac merged 1 commit into
feat/kits-fbe-sync-task-modulefrom
feat/kits-fbe-sync-queue-params
Sep 8, 2026
Merged

feat(firestore-bigquery-export): add the sync queue params#3129
cabljac merged 1 commit into
feat/kits-fbe-sync-task-modulefrom
feat/kits-fbe-sync-queue-params

Conversation

@cabljac

@cabljac cabljac commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Third layer of the Cloud Tasks write buffer stack. Adds MAX_DISPATCHES_PER_SECOND and MAX_ENQUEUE_ATTEMPTS with the extension's defaults of 100 and 3 and its validation ranges, exposes the first as a config expression for the queue's rate limit, and adds the matching optional ExportConfig fields with defaults applied in resolveExportConfig. Nothing reads the new fields yet.

One subtlety worth a look: IntParam.value() is parseInt(env || "0", 10) || 0 and never consults the declared default, so an unset param would reach resolveExportConfig as 0 and defeat the ?? 100 and ?? 3 fallbacks. optionalInt reports unset and blank env vars as undefined and passes an explicit 0 through as a real setting. The test mock for defineInt mirrors the real behaviour so the tests would catch a regression.

81 tests pass.

Part of #3031.

@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 introduces two new configuration parameters to the Firestore BigQuery Export extension: MAX_DISPATCHES_PER_SECOND and MAX_ENQUEUE_ATTEMPTS, along with corresponding tests and helper functions to handle their defaults. The review comments correctly point out that the validation regexes for both parameters do not allow empty strings, which would cause deployment-time validation failures if left blank. Modifying these regexes to allow empty values is recommended.

Comment thread kits/firestore-bigquery-export/src/config.ts
Comment thread kits/firestore-bigquery-export/src/config.ts
@cabljac
cabljac force-pushed the feat/kits-fbe-sync-queue-params branch from 713ed1d to bcc01ef Compare September 7, 2026 16:45
@cabljac
cabljac marked this pull request as ready for review September 7, 2026 16:50
@cabljac
cabljac force-pushed the feat/kits-fbe-sync-queue-params branch from bcc01ef to b3438b6 Compare September 7, 2026 16:52
@cabljac cabljac mentioned this pull request Sep 7, 2026
69 tasks
@cabljac
cabljac force-pushed the feat/kits-fbe-sync-queue-params branch from b3438b6 to d9f7bd9 Compare September 7, 2026 17:26
@cabljac
cabljac force-pushed the feat/kits-fbe-sync-queue-params branch from d9f7bd9 to 50d404a Compare September 8, 2026 09:06
@cabljac
cabljac requested a review from a team as a code owner September 8, 2026 10:03
@cabljac
cabljac force-pushed the feat/kits-fbe-sync-queue-params branch from 50d404a to d9f7bd9 Compare September 8, 2026 10:03
@cabljac
cabljac force-pushed the feat/kits-fbe-sync-queue-params branch 2 times, most recently from 0957a8e to f3efe84 Compare September 8, 2026 10:41

@IzaakGough IzaakGough 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.

Approving. Params, defaults and ranges match the extension, and optionalInt is guarding a real case: prepare.ts only copies a resolved param into the function env if (envValue && ...), so a blank is dropped and the var really is absent at runtime. tsc --noEmit is clean and the suite passes.

One non-blocking note on the two regex comments, inline. There is no runtime difference either way, so this can land as-is and be tidied in the PR that wires up rateLimits.

Comment thread kits/firestore-bigquery-export/src/config.ts Outdated
Comment thread kits/firestore-bigquery-export/src/config.ts
Adds MAX_DISPATCHES_PER_SECOND and MAX_ENQUEUE_ATTEMPTS with the
extension's defaults of 100 and 3 and its validation ranges, plus the
matching ExportConfig fields. IntParam.value() never consults the declared
default, so unset and blank values are reported as undefined and the
default applies in resolveExportConfig; an explicit 0 is preserved.

Nothing reads the new fields yet.
@cabljac
cabljac force-pushed the feat/kits-fbe-sync-queue-params branch from f3efe84 to 6fbe72f Compare September 8, 2026 10:49
@cabljac
cabljac merged commit 668669e into kits Sep 8, 2026
11 checks passed
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