You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This change deprecates the stream_snapshot: (true|false) config in favour for snapshot_mode which supports a range of enums, including snapshot_only which allows users to launch the connector, perform a snapshot and then shutdown without it automatically switching to streaming. This makes it easier for new tables to be loaded.
snapshot_mode now includes:
none (equivalent of stream_snapshot: false)
snapshot_only (currently not representable in existing config)
snapshot_and_stream (equivalent of stream_snapshot: true
Message format is correct throughout (oracledb_cdc: <imperative>), and docs commits are kept separate from code. Two quality/granularity issues:
Typos that hurt readability:
oracledb_cdc: mode snapshot_mode config parsing to dedicated file — "mode" should be "move"; as written the message is hard to parse.
oracledb_cdc: update batching to ensure we exist loop first — "exist" should be "exit".
Vague WIP-style commits that should be squashed:clean up, tidy up config, tidy up benchmark config, update comments. These are iterative refinements of the same feature rather than self-contained logical changes, and the messages are too vague to convey intent.
Review
Reviewed the snapshot_mode addition to oracledb_cdc (config parsing + backward-compat, the snapshot-only stop path, and the batcher.go flush/shutdown changes) against the CDC connector standard, the Go patterns, and the test patterns.
LGTM — no code issues found. Notable points confirmed as correct:
snapshot_mode enum (3 modes) is permitted by §5.3.1; stream_snapshot is retained and Deprecated() per §5.3.2; the SCN checkpoint is persisted as soon as the snapshot completes per §5.4.1.
snapshot_mode is intentionally Optional() (no Default) so the stream_snapshot: true backward-compat fallback in parseSnapshotMode still fires; a static default would silently break existing configs.
The batcher.go HardStopCtx/FlushRemaining/deferred-close changes and the snapshotOnlyDone + ErrEndOfInput path are internally consistent (final batch is delivered before TriggerHasStopped, no double-close, no deadlock with an active consumer).
Config parsing is unit-tested and snapshot-only is exercised by the updated concurrent-snapshot integration test.
A few message-quality issues (message quality is enforced); none block, but worth tidying before merge:
Vague messages that do not describe the change: oracledb_cdc: clean up and oracledb_cdc: tidy up config. State what was cleaned/tidied.
Typos: oracledb_cdc: mode snapshot_mode config parsing to dedicated file ("mode" → "move") and oracledb_cdc: update batching to ensure we exist loop first ("exist" → "exit").
The 15 commits are largely iterative fixups of a single feature (clean up / tidy up / update comments / update ctx handling). Consider squashing into a few self-contained logical commits. Documentation changes are already correctly isolated in their own commits (update docs, update snapshot_mode docs).
Review
Adds a snapshot_mode enum (none / snapshot_only / snapshot_and_stream) to oracledb_cdc, deprecating the boolean stream_snapshot. Reviewed against the §5 CDC Connector Standard and project Go patterns.
Config naming conforms: snapshot_mode is the canonical enum name and the 3 modes justify it over a boolean (§5.3.1); stream_snapshot is kept accepted and marked Deprecated() with backward-compat precedence handled in parseSnapshotMode (§5.3.2).
§5.4.1 satisfied: the SCN checkpoint is persisted as soon as the snapshot completes (input_oracledb_cdc.go:570), before the snapshot_only early-return.
The FlushRemaining / HardStopCtx / snapshotOnlyDone shutdown path is sound: the loop is fully stopped before the final partial-batch flush, and ReadBatch returns ErrEndOfInput only after the final batch is handed off.
Covered by an updated integration test (TestIntegrationOracleDBCDCConcurrentSnapshot now exercises snapshot_only) and a new TestParseSnapshotMode unit test including the backward-compat and precedence cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change deprecates the
stream_snapshot: (true|false)config in favour forsnapshot_modewhich supports a range of enums, includingsnapshot_onlywhich allows users to launch the connector, perform a snapshot and then shutdown without it automatically switching to streaming. This makes it easier for new tables to be loaded.snapshot_modenow includes:none(equivalent ofstream_snapshot: false)snapshot_only(currently not representable in existing config)snapshot_and_stream(equivalent ofstream_snapshot: true