feat(firestore-bigquery-export): add the sync queue params - #3129
Conversation
There was a problem hiding this comment.
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.
713ed1d to
bcc01ef
Compare
bcc01ef to
b3438b6
Compare
b3438b6 to
d9f7bd9
Compare
d9f7bd9 to
50d404a
Compare
50d404a to
d9f7bd9
Compare
0957a8e to
f3efe84
Compare
IzaakGough
left a comment
There was a problem hiding this comment.
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.
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.
f3efe84 to
6fbe72f
Compare
Third layer of the Cloud Tasks write buffer stack. Adds
MAX_DISPATCHES_PER_SECONDandMAX_ENQUEUE_ATTEMPTSwith 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 optionalExportConfigfields with defaults applied inresolveExportConfig. Nothing reads the new fields yet.One subtlety worth a look:
IntParam.value()isparseInt(env || "0", 10) || 0and never consults the declared default, so an unset param would reachresolveExportConfigas 0 and defeat the?? 100and?? 3fallbacks.optionalIntreports unset and blank env vars asundefinedand passes an explicit 0 through as a real setting. The test mock fordefineIntmirrors the real behaviour so the tests would catch a regression.81 tests pass.
Part of #3031.