oracledb_cdc: Support snapshot only mode, deprecating stream_snapshot config

[josephwoodward]

No description provided.

[claude]

Commits
LGTM — single commit oracledb_cdc: support snapshot_only mode follows the system: message convention (lowercase scope, imperative mood) and accurately describes the change.

Review
The snapshot_mode enum addition is well-formed and the snapshot_only stop path correctly persists the SCN checkpoint before stopping (§5.4.1). Three issues to address before merge:

1. stream_snapshot not marked Deprecated() — CDC Standard §5.3.2 requires the superseded field be marked Deprecated() (as pg_stream does); this PR uses a LintRule warning instead.
2. New snapshot_only mode is untested — the integration test changes are pure config renames; no test exercises the new stop-after-snapshot behavior or the stream_snapshot→snapshot_and_stream backward-compat path (§1.3.2–1.3.3).
3. Generated docs are stale — adding snapshot_mode changes the component page, but docs/modules/components/pages/inputs/oracledb_cdc.adoc was not regenerated; CI fails on stale docs (§6).

[claude]

Commits
LGTM

Review
This PR adds a snapshot_mode enum (none/snapshot_only/snapshot_and_stream) to oracledb_cdc, superseding the boolean stream_snapshot, with backward-compat upgrade logic and a snapshot-only flow that checkpoints the SCN before stopping. The enum (3 modes) is permitted by CDC Standard 5.3.1, and snapshot-only correctly persists the checkpoint on completion (5.4.1). Two issues:

1. 5.3.2 — superseded field not marked Deprecated() (input_oracledb_cdc.go#L121-L123): stream_snapshot only gets a LintRule. The standard requires .Deprecated() (precedent: the snapshot_memory_safety_factor field in postgres_cdc).

2. Backward-compat precedence (input_oracledb_cdc.go#L279-L286): the stream_snapshot fallback fires whenever the resolved mode is none, including when the user explicitly sets snapshot_mode: none, silently upgrading to snapshot_and_stream and contradicting the explicit setting. Use conf.Contains() to detect whether snapshot_mode was actually provided.

[claude]

Commits

Two minor commit-message quality issues (STEP 1 message-quality is enforced):

1. d3260bd — oracledb_cdc: mode snapshot_mode config parsing to dedicated file contains a typo: mode should be move.
2. 8b26fe9 — oracledb_cdc: clean up is vague; the message should describe what was cleaned up. Along with the two docs commits (81708564, 61c78605) and the move/clean-up commits, this history reads like iterative fixups that would benefit from squashing into the logical changes they refine.

Message format is otherwise valid (system: imperative message), and docs changes are correctly separated from code changes.

Review

The change migrates oracledb_cdc from the stream_snapshot bool to a snapshot_mode enum (none / snapshot_only / snapshot_and_stream). It is well-implemented and conformant with the CDC Connector Standard:

• The enum is justified per §5.3.1 (three distinct modes), and stream_snapshot is kept accepted and marked Deprecated() — the non-breaking rename pattern required by §5.3.2.
• Backward compatibility is handled correctly: snapshot_mode is Optional() with no default so Contains() can distinguish an explicit value from the stream_snapshot fallback (a default would have broken this).
• snapshot-only mode persists the SCN checkpoint (cacheSCN) before stopping, satisfying §5.4.1 (checkpoint as soon as the snapshot completes).
• Unit (TestParseSnapshotMode) and integration coverage added.

LGTM.

[claude]

Commits

1. oracledb_cdc: mode snapshot_mode config parsing to dedicated file — typo: mode should read move. Minor, but the message reads as garbled; worth fixing before merge.

Otherwise commit hygiene is good: messages follow system: message format, imperative mood, and docs changes are isolated in their own commits.

Review
The change deprecates stream_snapshot in favour of a three-valued snapshot_mode enum, keeping the old field accepted and marked Deprecated(). This conforms to the CDC standard for non-breaking field renames (CONTRIBUTING §5.3.2) and the multi-mode enum is justified (§5.3.1). The SCN checkpoint is still persisted as soon as the snapshot completes (§5.4.1), and the backward-compat fallback is well covered by TestParseSnapshotMode.

One functional issue:

1. snapshot_only mode does not terminate the pipeline — it triggers a stop that makes ReadBatch return service.ErrNotConnected, which the framework treats as a reconnect signal, causing an infinite reconnect loop instead of a clean shutdown. See inline comment on input_oracledb_cdc.go#L576-L580.

[claude]

Commits

LGTM — all commits follow the oracledb_cdc: <imperative> convention, and docs are correctly split into separate commits from code. Minor: oracledb_cdc: mode snapshot_mode config parsing to dedicated file has a typo ("mode" → "move"), and a few messages ("clean up", "tidy up config") are on the vague side. These look like fixup-style commits that would benefit from being squashed before merge, but none are policy violations.

Review

This PR replaces the boolean stream_snapshot field with a three-valued snapshot_mode enum (none / snapshot_only / snapshot_and_stream) on the oracledb_cdc input, keeping stream_snapshot accepted and Deprecated(). The migration approach is correct and conforms to the CDC standard (§5.3.1 allows the enum since there are >2 modes; §5.3.2 deprecation-with-backward-compat is followed), and the snapshot checkpoint is still persisted before stopping (§5.4.1).

2 issues flagged inline:

1. Data completeness in snapshot_only mode (input_oracledb_cdc.go:579-584): the goroutine stops immediately after the snapshot with no batcher flush/drain. With a user-configured batching count > 1, the trailing partial batch is dropped, breaking snapshot completeness / at-least-once delivery.
2. Incorrect doc comment (config.go:118): IsSnapshotOnly()'s godoc names the wrong method (IsSnapshotNone) and both new methods say "true of" instead of "true if".

[claude]

Commits

1. Commit oracledb_cdc: mode snapshot_mode config parsing to dedicated file has a typo — "mode" should be "move", which makes the message read as incomprehensible. Minor; otherwise messages follow the system: message format and are imperative/lowercase.

Review

The PR replaces stream_snapshot (bool) with a snapshot_mode enum (none/snapshot_only/snapshot_and_stream) on oracledb_cdc, keeping stream_snapshot accepted-but-Deprecated() for backward compat. This conforms to the CDC standard: enum is justified by >2 modes (§5.3.1), the rename keeps the old field accepted and deprecated (§5.3.2), and the snapshot checkpoint is persisted as soon as the snapshot completes (§5.4.1, cacheSCN before stopping). Backward-compat parsing is well covered by TestParseSnapshotMode.

1. Possible data-loss race in snapshot_only shutdown: FlushRemaining synchronizes only its own send, but the publisher loop() goroutine is a competing publisher on the unbuffered msgChan. With period-based batching (the config this PR adopts in bench/benchmark_config.yaml), a timed flush from loop() can be in flight when TriggerHasStopped() fires, and ReadBatch's select may pick the stop branch over the pending batch, dropping the last batch via ErrEndOfInput. See inline comment.

[claude]

This config sets both the now-deprecated stream_snapshot: true and the new snapshot_mode: snapshot_and_stream. Everywhere else in this PR stream_snapshot was cleanly replaced by snapshot_mode (e.g. the lob_enabled: false subtest just above at line 795), so leaving both here looks like an incomplete edit and is confusing (which one wins?). Drop the deprecated stream_snapshot: true line.

[claude]

Commits

Format and scope are fine (all oracledb_cdc: scoped, lowercase imperative, docs split into their own commits). A few hygiene issues:

1. Several commits are iterative refinements of earlier commits in the same PR rather than self-contained logical changes, and should be squashed: oracledb_cdc: clean up, oracledb_cdc: tidy up config, oracledb_cdc: tidy up benchmark config, oracledb_cdc: update comments, oracledb_cdc: update batching to ensure we exist loop first.
2. Vague message: oracledb_cdc: clean up does not describe the change.
3. Typos: mode snapshot_mode config parsing to dedicated file (should be "move"); update batching to ensure we exist loop first (should be "exit").

Review

The migration from stream_snapshot (bool) to snapshot_mode (enum) is well executed: the new enum genuinely has three modes so it is permitted under the CDC standard (§5.3.1), the old field is kept and marked Deprecated() for non-breaking backward compatibility (§5.3.2), backward-compat precedence is unit-tested, and the snapshot-only path still checkpoints the SCN as soon as the snapshot completes (§5.4.1). The batcher FlushRemaining/HardStopCtx reasoning is sound and well documented.

1. internal/impl/oracledb/integration_test.go L885-L886 — a single test config sets both the deprecated stream_snapshot: true and snapshot_mode: snapshot_and_stream; looks like an incomplete edit (the deprecated line should be removed, as it was elsewhere in the PR).

[claude]

Commits

1. Vague messages that do not describe the change: oracledb_cdc: clean up, oracledb_cdc: tidy up config, oracledb_cdc: update comments, oracledb_cdc: update ctx handling. The enforced commit policy requires messages to describe what changed.
2. Typos affecting clarity: 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).
3. Granularity (CONTRIBUTING §3.3.2): several of the 15 commits are incremental fixups of the same feature (clean up, tidy up config, tidy up benchmark config, add version to new config, remove lint rule, update comments, update ctx handling) and should be squashed into self-contained logical commits.

Review
Adds a snapshot_mode enum (none / snapshot_only / snapshot_and_stream) to oracledb_cdc, deprecating stream_snapshot with a backward-compatible fallback, plus snapshot-only termination via service.ErrEndOfInput. This conforms to the CDC standard: §5.3.2 (add the canonical field, keep the old one accepted and Deprecated()) and §5.4.1 (SCN checkpoint persisted as soon as the snapshot completes). The batcher soft/hard-stop handling and the new FlushRemaining flow were reviewed (ordering of the final flush, ReadBatch consume, and checkpoint persistence) and look correct.

LGTM

[josephwoodward]

Closing this to remove a lot of the noise from the PR, will open a new PR.