Repository navigation
gate for obj fast ingest - #397
Conversation
|
Claude finished @aricart's task in 3m 3s —— View job PR Review: gate for obj fast ingest
SummaryThe PR flips fast ingest from opt-out to opt-in (hidden gate via an undocumented Correctness concern (moderate): API asymmetry (low): The Discoverability (informational): The Test nit: The |
![synadia-claude-reviewer[bot]](https://avatars.githubusercontent.com/u/132956691?s=60&v=4){kind=small}
| if (info === null) { | ||
| return Promise.reject(new Error("object store not found")); | ||
| } | ||
| this.supportsFastIngest = this.fiRequested && |
There was a problem hiding this comment.
status() now has a hidden side-effect on supportsFastIngest. This creates a subtle footgun for the open(check=false, fastIngest=true) path: that path explicitly bypasses the probe and sets supportsFastIngest = true unconditionally, but a later call to status() (e.g. for monitoring) will silently flip the flag back to false if allow_batched isn't actually set on the stream — silently breaking the "trust the caller" contract.
Either document this re-evaluation behavior, or guard against it by not modifying supportsFastIngest when the caller explicitly bypassed the check.
| } else { | ||
| const o = check as Record<string, unknown>; | ||
| doCheck = o.check !== false; | ||
| fastIngest = o.fastIngest === true; |
There was a problem hiding this comment.
The open() type signature (boolean | { check?: boolean }) doesn't expose fastIngest at all. Passing { check: true, fastIngest: true } satisfies { check?: boolean } at runtime, but nothing in the type surface signals to callers that fastIngest is a recognized option here. Consider either adding an explicit internal type (even if unexported) or adding a JSDoc note so the intent is discoverable during code review.
| } | ||
| } else if (fastIngest) { | ||
| // no probe — trust caller | ||
| os.supportsFastIngest = true; |
There was a problem hiding this comment.
The check=false path skips both the stream capability probe and the api_lvl >= 4 server check that init() enforces. If a caller opens with {check: false, fastIngest: true} against a server that doesn't support batched publishes, writes will fail at runtime with a protocol-level error rather than a clean early rejection. The asymmetry with create() (which always checks apiLvl < 4) may be intentional but is worth a comment.
|
|
||
| // hidden opt-in for fast-ingest. property is not on ObjectStoreOptions; the | ||
| // implementation reads it via cast and strips it before stream config build. | ||
| function fi( |
There was a problem hiding this comment.
The fi() helper is defined between two import blocks (the import from objectstore.ts comes before it, the import from base64.ts comes after). This splits the import section in a surprising way. Minor: move fi() after all imports, or move both import statements above it.
#391) * feat(jsapi): add `ConsumerResetResponse` and reset functionality for consumer state management - `reset()` and `resetConsumer()` methods for resetting consumer delivery state. --------- Signed-off-by: Alberto Ricart <alberto@synadia.com>
|
@claude re-review |
|
Claude finished @aricart's task in 2m 10s —— View job PR Re-Review: gate for obj fast ingest
SummaryAll four concerns from the previous review have been addressed. Here's the breakdown: Previous concerns — status1.
2.
3. The option was renamed from 4. The New observationsRename is clean.
One very minor nit: the JSDoc on OverallThis is a clean, well-reasoned revision. The gate pattern (opt-in, hidden from public types, server capability checked eagerly on both code paths) is correctly implemented. No blocking issues. |
No description provided.