Skip to content

Connector couchbase migration - #12076

Open
KaustAbhinand wants to merge 12 commits into
apache:devfrom
KaustAbhinand:connector-couchbase-migration
Open

Connector couchbase migration#12076
KaustAbhinand wants to merge 12 commits into
apache:devfrom
KaustAbhinand:connector-couchbase-migration

Conversation

@KaustAbhinand

@KaustAbhinand KaustAbhinand commented Sep 3, 2026

Copy link
Copy Markdown

Purpose of this pull request

This PR performs the migration of CouchbaseWriter to engine level timer flush. Unit tests and E2E tests have been added in addition to ensure the new code is correct.

Closes #12016

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit tests have been added in CouchbaseWriter.java and E2E tests have been added in CouchbaseIT.java

Check list

@KaustAbhinand

Copy link
Copy Markdown
Author

Hi @nzw921rx! I have completed the migration to engine level timer flush. Please review the code.

@@ -0,0 +1,41 @@
env {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add license

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

What Problem Does This PR Solve?

The Couchbase sink previously enforced its buffer-flush.interval max-latency guarantee with a connector-owned ScheduledExecutorService background thread, plus an AtomicReference-latched async-error mechanism so write/prepareCommit/close could detect and rethrow a failure from that background thread. This PR migrates CouchbaseWriter to the engine-level timer-flush mechanism (SinkWriter.Context#registerFlushAction, STIP-23) already adopted by ClickHouse, Doris, Elasticsearch, Hudi, JDBC, MongoDB, Prometheus, and StarRocks: on Zeta the engine now delivers a FlushSignal through the normal task-processing thread instead of a separate timer thread, removing the connector-owned scheduler, its shutdown/interrupt handling, and the async-error latch entirely, while keeping the size-based (buffer-flush.max-rows), checkpoint-based (prepareCommit), and close-based flush paths unchanged.

1. Code Change Review

1.1 Core Logic Analysis

seatunnel-connectors-v2/connector-couchbase/.../sink/CouchbaseWriter.java constructor (previously ~40 lines building a daemon ScheduledExecutorService with scheduleAtFixedRate, now):

// Opt in to engine-level timer flush. On Zeta the engine invokes this action on the normal
// Sink input-processing path when a FlushSignal arrives, so there is no connector-owned
// scheduler thread and no concurrency with write/checkpoint/close. On Spark and Flink the
// Context does not implement registerFlushAction (it keeps the interface's no-op default),
// so there is no periodic timer flush there; the buffer is flushed on buffer-flush.max-rows,
// on checkpoint, and on close(). The null-check is defensive for non-standard/test call
// sites that may not supply a context.
if (context != null) {
    context.registerFlushAction(this::doFlush);
}

close() is correspondingly simplified from ~35 lines (cancel timer, shutdownNow, awaitTermination(30s), latch check) down to a plain doFlush() inside a try/finally that still disconnects the cluster and still attaches a disconnect failure as a suppressed exception on top of a primary flush failure.

Key findings:

  1. The engine-level FlushSignal dispatch was independently verified, not just trusted from the PR's comment. I traced SinkWriterContext.registerFlushAction/getFlushAction (seatunnel-engine-server/.../task/context/SinkWriterContext.java:102) into SinkFlowLifeCycle.processSignal (.../task/flow/SinkFlowLifeCycle.java:728-736), which is called only from SinkFlowLifeCycle.received(Record<?> record) (line 267) — the single entry point that also dispatches processCheckpointBarrier and processDataRecord on the same call stack. A FlushSignal therefore arrives as a Record through the normal upstream-to-downstream delivery path and is processed serially with every other record/barrier on the task's own thread — there genuinely is no separate scheduler thread and no concurrency between the flush action and write/prepareCommit/close on Zeta, exactly as the new comment claims. This is not novel infrastructure invented by this PR; it is the same proven mechanism eight other connectors already rely on.
  2. Because document ids are still assigned at write() time (unchanged logic, WriteUnit(buildDocumentKey(doc), doc)), and doFlush() remains synchronized with the startFrom/ambiguousIndices retry bookkeeping intact, removing the timer thread does not reopen the duplicate-document / spurious-DocumentExistsException problem that the surrounding Javadoc (lines 72-78) describes solving — that protection was about buffer re-flush ordering, not about which thread calls doFlush(), so it survives the migration correctly.
  3. The class-level Javadoc (lines 51-61) was not updated and is now factually wrong. It still reads: "A periodic background timer fires every buffer-flush.interval milliseconds (the real max-latency guarantee, enforced even when no new rows arrive)" — but buffer-flush.interval no longer exists as a config option (removed from CouchbaseSinkOptions/CouchbaseSinkFactory in this same PR), and there is no more connector-owned timer. This is a direct, in-file contradiction between the class Javadoc and the constructor comment 80 lines below it, which correctly describes the new engine-driven mechanism. See Issue 3.
  4. A dangling orphaned Javadoc block was left behind by the refactor. Lines 236-240 (current file) read:
    /**
     * Throws a {@link CouchbaseConnectorException} wrapping the latched async error if one has
     * been recorded by the background flush timer.
     */
    immediately followed by the Javadoc for toJsonObject. The checkAsyncFlushError() method this comment used to document was deleted along with its callers, but the comment block itself was not removed. See Issue 4.
  5. prepareCommit() and write() had their checkAsyncFlushError() guard calls removed, which is correct and necessary (the method they called no longer exists), but this also means the checkpoint-time (prepareCommit) contract is now simpler and, if anything, more correct: previously a checkpoint could fail because of an unrelated background-thread error surfacing at just the wrong moment; now prepareCommit() only ever fails because of its own synchronous doFlush() call, which is a strictly more predictable failure surface for the engine's checkpoint machinery to reason about.

Runtime path (Zeta, streaming job with sink.flush.interval set in env):
FakeSource/upstream operator emits rows → SeaTunnelRow records flow to SinkFlowLifeCycle.received()processDataRecordCouchbaseWriter.write(row) buffers a WriteUnit (synchronized(this)) → in parallel, the engine's periodic timer (outside this connector, existing STIP-23 core infra) injects a FlushSignal Record into the same task input stream at sink.flush.intervalSinkFlowLifeCycle.received() routes it to processSignalwriterContext.getFlushAction().run()CouchbaseWriter.doFlush() (same synchronized(this) monitor as write, same thread as all record processing) → buffered WriteUnits are upsert/insert-ed to the Couchbase Collection → buffer cleared. On checkpoint, SinkFlowLifeCycle.processCheckpointBarrier still drives prepareCommit()doFlush() through the same synchronous path, unaffected by the timer-flush migration. On job close, close() still does a final synchronous doFlush() before cluster.disconnect(), exactly as before.

1.2 Compatibility Impact

Fully compatible, verified with direct evidence rather than assumed. The removed buffer-flush.interval option (and its Builder.withBatchIntervalMs/batchIntervalMs field) looks at first glance like a breaking config removal, which would normally be a hard blocker per this project's backward-compatibility rules. I checked git log --oneline --all for CouchbaseSinkOptions.java: it has exactly two commits — the connector's original addition (ead62107e3, "[Feature][Connector-V2] Add Couchbase Sink Connector (#11198)") and a follow-up fix (a0ce9cbf13). Neither commit is reachable from any released tag (git tag --contains ead62107e3 returns nothing; the most recent tag is 2.3.13). The Couchbase sink connector, and therefore buffer-flush.interval, has never shipped in a released version of Apache SeaTunnel — there is no production config in the wild that this removal can break, so this is not an incompatible change in the sense the project's compatibility rules are protecting against, and no incompatible-changes.md entry is warranted. This reasoning should be stated explicitly in the PR description, since a reviewer who does not check tag reachability would reasonably flag this as a hard blocker.

1.3 Performance / Side-Effect Analysis

Net positive. This removes a per-writer-instance daemon thread (Executors.newSingleThreadScheduledExecutor) and its associated shutdown/awaitTermination(30s)/interrupt-handling code entirely — for a job with N parallel Couchbase sink subtasks, that is N fewer long-lived threads and N fewer 30-second worst-case shutdown waits on every close(). The synchronized keyword on doFlush() and the synchronized(this) block in write() are now technically redundant given the verified single-threaded engine dispatch (finding 1 above), but retaining them is harmless (an uncontended intrinsic lock is cheap) and provides defensive depth for the Spark/Flink paths, where registerFlushAction is a no-op and multiple threads could in principle still call into this writer depending on those engines' own threading model — I would not ask the author to remove this locking.

1.4 Error Handling and Logging

The suppressed-exception handling in close() (flush failure as primary cause, disconnect failure attached via addSuppressed) is preserved unchanged and correctly reasoned about, per the existing CouchbaseWriterCloseTest. See Issues 1-5 below for the concrete problems found in this area.

Number Issue Location Problem Potential risk Best improvement Severity Raised by another reviewer
1 seatunnel-connectors-v2/connector-couchbase/src/test/java/.../sink/CouchbaseWriterTest.java (new file, whole file) New file has no ASF license header at all (starts directly with package ...); the sibling new resource seatunnel-e2e/.../fake_source_to_couchbase_timer_flush.conf is also missing the standard #-comment license header that its sibling fake_source_to_couchbase.conf correctly carries. Confirmed via the actual fork CI run (KaustAbhinand/seatunnel run 33797345126, job Run / License header, 100788115908): ERROR the following files don't have a valid license header: seatunnel-connectors-v2/connector-couchbase/src/test/java/.../CouchbaseWriterTest.java, seatunnel-e2e/.../fake_source_to_couchbase_timer_flush.conf. The apache-side Build check for this PR is currently failure because of exactly this; every downstream CI job (unit-test, engine-v2-it, all E2E buckets) is skipped as a result, so nothing past License header has actually run yet — see Issue 2, which this failure is currently masking from CI. Add the standard ASF Java-comment license header to CouchbaseWriterTest.java and the standard #-comment header to fake_source_to_couchbase_timer_flush.conf, matching every other file in these two directories. High No
2 seatunnel-connectors-v2/connector-couchbase/src/test/java/.../sink/CouchbaseWriterTest.java, all 5 @Test methods Every test configures minimalOptions(), whose builder never calls .withUpsertEnable(true)CouchbaseWriterOptions.Builder.upsertEnable defaults to false (CouchbaseWriterOptions.java:73). This means CouchbaseWriter.doFlush() (line 581, unchanged by this PR: if (options.isUpsertEnable()) { collection.upsert(...) } else { collection.insert(...) }) takes the insert branch, not the upsert branch, on every one of these tests. But every test stubs and/or verifies collection.upsert(...), never collection.insert(...): when(collection.upsert(...)).thenReturn(...) (tests 1/3/4), doThrow(...).when(collection).upsert(...) (tests 2/5), and verify(collection, times(1)).upsert(...) (tests 1/3/4). Since the actual code path is collection.insert(...), which is an unstubbed mock call returning null with no exception, I traced each test to its actual outcome: Test 1 (shouldRegisterFlushActionAndFlushBufferedRecordsOnSignal) — doFlush() completes silently via insert, then verify(collection, times(1)).upsert(...) fails with "Wanted but not invoked". Test 2 (shouldPropagateFlushFailure) — the stubbed upsert throw never fires, doFlush() succeeds, so Assertions.assertThrows(CouchbaseConnectorException.class, ...) fails because no exception is thrown. Test 3 (shouldFlushOnCloseWhenEngineNeverInvokesFlushAction) — same upsert-verification failure as test 1. Test 4 (shouldFlushOnPrepareCommitWhenEngineNeverInvokesFlushAction) — same upsert-verification failure as test 1. Test 5 (closeShouldKeepFlushExceptionWhenDisconnectAlsoThrows) — the stubbed upsert throw never fires so doFlush() succeeds in close()'s try, primaryThrowable stays null, the separately-stubbed cluster.disconnect() throw then becomes the sole thrown exception (wrapped as CLOSE_CLIENT_FAILED) rather than a suppressed addition to a flush failure — so thrown.getSuppressed() is empty and the test's own assertTrue(disconnectSuppressed, ...) fails. All 5 tests are expected to fail on first execution once License header (Issue 1) is fixed and the unit-test CI job actually runs. This is not a flaky or edge-case test; it is a deterministic, first-run failure. It is currently invisible because CI never reaches the unit-test job (blocked upstream by Issue 1), so the author has not seen these tests actually execute. If merged as-is (once Issue 1 is separately fixed), CI would go red on unit-test for connector-couchbase, and the intended regression coverage for the exact flush paths this PR restructures (signal-driven flush, close-fallback flush, checkpoint flush, suppressed-exception-on-close) would not exist in a passing state. Add .withUpsertEnable(true) to minimalOptions() (or to each test that needs upsert semantics specifically), so the stubbed/verified collection.upsert(...) calls match the code path actually taken; alternatively, if insert semantics are what the author intended to test, restub/verify against collection.insert(...) instead. Either fix is small; the current mismatch appears to be a straightforward oversight rather than a design question. High No
3 seatunnel-connectors-v2/connector-couchbase/src/main/java/.../sink/CouchbaseWriter.java, class-level Javadoc, lines 51-61 The class Javadoc still documents the removed connector-owned timer: "A periodic background timer fires every buffer-flush.interval milliseconds (the real max-latency guarantee, enforced even when no new rows arrive)"buffer-flush.interval was removed from CouchbaseSinkOptions by this same PR, and there is no more background timer; the actual mechanism is now the engine-level FlushSignal correctly described 80 lines later in the constructor's inline comment. A future maintainer reading only the class Javadoc (the first thing anyone opens this file to read) gets a description of a mechanism that no longer exists in this class, including a config key that no longer parses. Update the class Javadoc's bullet list to describe the engine-level FlushSignal/registerFlushAction mechanism (Zeta) versus the size/checkpoint/close-only fallback (Spark/Flink), consistent with the constructor's own comment. Medium No
4 seatunnel-connectors-v2/connector-couchbase/src/main/java/.../sink/CouchbaseWriter.java, lines 236-240 An orphaned Javadoc block — "Throws a {@link CouchbaseConnectorException} wrapping the latched async error if one has been recorded by the background flush timer." — was left behind after the checkAsyncFlushError() method it documented was deleted; it now sits between the "Internal helpers" section header and the toJsonObject Javadoc, describing nothing. Dead, misleading documentation; a reader will spend time looking for the method this comment describes. Delete the orphaned comment block as part of the same refactor that removed checkAsyncFlushError(). Low No
5 docs/en/connectors/sink/Couchbase.md and docs/zh/connectors/sink/Couchbase.md (neither touched by this PR); also seatunnel-e2e/seatunnel-connector-v2-e2e/connector-couchbase-e2e/src/test/resources/fake_source_to_couchbase.conf:58 (pre-existing, not cleaned up) docs/en/connectors/sink/Couchbase.md:84 still documents buffer-flush.interval as a valid option ("Maximum milliseconds between batch writes... default 30000") and shows it in the example config at lines 163-164; docs/zh/connectors/sink/Couchbase.md has the identical stale entry. The connector no longer recognizes this key at all (confirmed: it is not present in CouchbaseSinkFactory's optional(...) list after this PR, and SeaTunnel's TableSinkFactory/OptionRule validation does not reject unrecognized config keys, it simply ignores them — I checked seatunnel-api/.../configuration/util/ConfigUtil.java for an "unrecognized option" rejection path and found none for this case). Separately, the pre-existing E2E resource fake_source_to_couchbase.conf:58 still sets buffer-flush.interval = 5000, which is now a silently-ignored dead key in a test that this PR did not update. Neither the [ ] timer flush feature checkbox nor a "Timer flush on Zeta" explanation section (the pattern this project already established for Doris/ClickHouse/Hudi/StarRocks/Elasticsearch/MongoDB — see docs/en/connectors/sink/Doris.md:24,198-210 for sink.flush.interval in the env block) were added to Couchbase.md. A user who follows the current, unmodified Couchbase.md docs and sets buffer-flush.interval in their config gets it silently ignored with zero warning or error — they will reasonably believe they have a max-latency guarantee they do not actually have. This directly conflicts with this project's stated rule that "Config names, defaults, and examples MUST match the code exactly." Update docs/en/connectors/sink/Couchbase.md and docs/zh/connectors/sink/Couchbase.md to remove the buffer-flush.interval row/example and add the established timer-flush feature checkbox plus a "Timer flush on Zeta" section documenting sink.flush.interval (mirroring Doris.md); also drop the now-dead buffer-flush.interval = 5000 line from fake_source_to_couchbase.conf. Medium No

2. Code Quality Assessment

2.1 Coding Standards

Mostly good — the constructor's new comment and the retry/dedup logic Javadocs (doFlush, buildDocumentKeyFrom) are thorough and explain why, not just what. The two documentation defects (Issues 3 and 4) are the concrete exceptions to this, both being leftovers of an otherwise clean refactor rather than missing documentation on new logic. No new core method or nontrivial field lacks a comment; the gap is stale/orphaned comments on removed logic, not absent comments on new logic.

2.2 Test Coverage and Test Stability

High risk. This is a formal rating, not just a comment on Issue 2: the entire new CouchbaseWriterTest.java — all 5 tests intended to cover exactly the behavior this PR restructures (signal-driven flush, Spark/Flink close-fallback flush, checkpoint flush, suppressed-exception-on-close) — will fail deterministically on first execution due to the upsert/insert mismatch described in Issue 2, once Issue 1 (missing license headers, currently blocking CI from reaching the unit-test job) is fixed. This is not a timing-related flake; it is a guaranteed failure masked only by an unrelated, earlier CI failure. The new E2E test (CouchbaseIT.testCouchbaseSinkTimerFlush) is, by contrast, well-constructed: it correctly sets upsert-enable = true in fake_source_to_couchbase_timer_flush.conf, uses sink.flush.interval = 3000 with a checkpoint.interval of 5 minutes so the assertion can only be satisfied by the timer-flush path (not an incidental checkpoint), and I confirmed via FakeSourceReader.pollNext (connector-fake/.../source/FakeSourceReader.java) that a STREAMING-mode FakeSource with a fixed row.num does not call context.signalNoMoreElement() once its splits are exhausted (that only fires under Boundedness.BOUNDED), so the job legitimately keeps running after emitting its 10 rows — the test's assertFalse(jobFuture.isDone(), ...) premise is sound, not a race.

2.3 Documentation Updates

Needs updates that were not made. See Issue 5: docs/en/connectors/sink/Couchbase.md and docs/zh/connectors/sink/Couchbase.md are stale with respect to both the removed buffer-flush.interval option and the new timer-flush capability, and are inconsistent with the documentation pattern already established for every other connector that has gone through this same STIP-23 migration.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix, applying a proven, already-adopted engine mechanism (STIP-23 registerFlushAction) rather than inventing anything new — this is exactly the kind of migration that should shrink connector-owned complexity, and it does: a ~40-line scheduler-lifecycle block and a full async-error-latch mechanism are replaced by one if (context != null) { context.registerFlushAction(this::doFlush); }. The two leftover documentation defects (Issues 3, 4) and the missing doc updates (Issue 5) are refactor-cleanup gaps, not design problems with the approach itself.

3.2 Maintainability

Improved by the migration itself (less connector-owned thread-lifecycle code to reason about), but currently undermined by the broken new test suite (Issue 2): a maintainer who trusts a green CouchbaseWriterTest in the future would be trusting a suite that, as written, cannot pass, which is worse for long-term maintainability than having no suite at all if it goes in broken and is later "fixed" by weakening assertions rather than fixing the root mismatch.

3.3 Extensibility

Neutral-to-positive; this brings Couchbase in line with the same registerFlushAction contract other sinks already implement, which is good for anyone building shared tooling or tests around that contract in the future (e.g. the existing MultiTableSinkWriterTest failure-policy tests in seatunnel-api).

3.4 Historical-Version Compatibility

Confirmed fully compatible — see 1.2. The Couchbase sink connector has never appeared in a released Apache SeaTunnel tag, so removing buffer-flush.interval does not break any production config, and no incompatible-changes.md entry or migration guidance is required.

4. Issue Summary

Number Issue Location Severity
1 Two new files (CouchbaseWriterTest.java, fake_source_to_couchbase_timer_flush.conf) are missing the mandatory ASF license header, currently failing the fork's CI License header job and blocking every downstream CI job (unit-test, E2E) from running at all CouchbaseWriterTest.java; fake_source_to_couchbase_timer_flush.conf High
2 All 5 new unit tests in CouchbaseWriterTest.java verify/stub collection.upsert(...), but minimalOptions() leaves upsertEnable at its false default, so the code actually calls collection.insert(...) — every test will fail deterministically once CI reaches the unit-test job CouchbaseWriterTest.java (all 5 tests); root cause in minimalOptions() High
5 docs/en/connectors/sink/Couchbase.md and docs/zh/connectors/sink/Couchbase.md still document the removed buffer-flush.interval option and were not updated with the new timer-flush capability, inconsistent with the pattern used by every other connector migrated under STIP-23; a stale, now-dead buffer-flush.interval = 5000 line also remains in the pre-existing E2E resource fake_source_to_couchbase.conf docs/en/connectors/sink/Couchbase.md; docs/zh/connectors/sink/Couchbase.md; fake_source_to_couchbase.conf:58 Medium
3 Class-level Javadoc on CouchbaseWriter still describes the removed connector-owned background timer and the removed buffer-flush.interval option CouchbaseWriter.java lines 51-61 Medium
4 Orphaned Javadoc block left over from the deleted checkAsyncFlushError() method CouchbaseWriter.java lines 236-240 Low

5. Merge Recommendation

Conclusion: Not recommended for merge

  1. Blockers — must be fixed (sorted by severity):
    • Issue 1: add the missing ASF license headers so CI can actually run past the License header job — this is currently the only thing standing between this PR and CI actually exercising its own new tests.
    • Issue 2: fix the upsert/insert mismatch in CouchbaseWriterTest.java (either add .withUpsertEnable(true) to minimalOptions(), or restub/verify against collection.insert(...) if insert semantics were actually intended) — as written, this test class cannot pass and would go red the moment Issue 1 is fixed and the unit-test job actually runs.
  2. Recommended fixes — non-blocking:
    • Issue 5: update docs/en/connectors/sink/Couchbase.md and docs/zh/connectors/sink/Couchbase.md to drop buffer-flush.interval and document the new timer-flush mechanism (sink.flush.interval) following the Doris.md pattern; clean up the dead buffer-flush.interval = 5000 line in fake_source_to_couchbase.conf.
    • Issue 3: update the CouchbaseWriter class Javadoc to describe the engine-level flush mechanism instead of the removed background timer.
    • Issue 4: delete the orphaned checkAsyncFlushError Javadoc block.

Overall assessment: the migration itself is architecturally sound and correctly executed — I independently traced the engine's FlushSignal/registerFlushAction dispatch path to confirm the single-threaded, no-concurrency claim in the new code comment actually holds, and confirmed via tag-reachability that the removed buffer-flush.interval option never shipped in a release, so there is no real backward-compatibility break despite the config removal. However, this PR is not ready to merge as-is: its own new CI run is currently red on a trivial, easily-fixed license-header issue (Issue 1), and that failure is currently hiding a second, more substantive problem — its own new unit test suite is written against the wrong Couchbase SDK call (upsert instead of the insert the code actually takes with default options) and would fail outright the moment CI reaches it. I don't see a better alternative implementation to the migration approach itself (it correctly reuses proven, existing engine infrastructure); the recommended fixes are all mechanical corrections to this PR's own new files, not design changes. Welcome to the project, and thank you for taking on a real, occasionally-tricky migration (the retry/ambiguous-timeout bookkeeping you preserved from the original PR is genuinely careful work) — the two blockers above are both small, quick fixes, and I'd be glad to take another look as soon as they're in.

@davidzollo davidzollo added the First-time contributor First-time contributor label Sep 4, 2026
@KaustAbhinand

Copy link
Copy Markdown
Author

Hello @nzw921rx! I have seen your review of the PR, please do not unassign it. I am making the changes, and will commit them once they are completed.

@KaustAbhinand

Copy link
Copy Markdown
Author

I have fixed the tests and added the ASF licence.

@KaustAbhinand

Copy link
Copy Markdown
Author

Hi @nzw921rx and @DanielLeens, I have fixed the changes mentioned in the review. Please go through it now.

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

Thanks for the update, @KaustAbhinand — I've re-reviewed the latest head (e9a0bd5377d). This is a full re-review, not an incremental diff-only pass: I re-verified all 5 issues from my previous round against the current code, and also read the full new commits end-to-end for anything new. Good news first: three of my five previous blockers/notes are genuinely fixed, and I independently re-traced the fix for the trickiest one (the upsert/insert test mismatch) rather than just trusting the commit message. One new problem was introduced by the fix commits themselves, and I also owe you an apology — I missed a third instance of the same "orphaned Javadoc" pattern in my first pass, in a file I hadn't flagged before. Details below.

What Problem Does This PR Solve?

This PR migrates the Couchbase sink's max-latency flush guarantee from a connector-owned ScheduledExecutorService background timer (with an AtomicReference-latched async-error mechanism) to the engine-level SinkWriter.Context#registerFlushAction timer-flush mechanism (STIP-23) already used by ClickHouse, Doris, Elasticsearch, Hudi, JDBC, MongoDB, Prometheus, and StarRocks. On Zeta, the engine now delivers a FlushSignal on the normal task-processing thread instead of a separate timer thread, eliminating the connector-owned scheduler and its shutdown/interrupt handling. Since my last review, this round's commits (1) added the missing ASF license headers, (2) fixed the mock/verify mismatch in the new unit test suite, (3) removed the dead buffer-flush.interval config from both docs and added a "Timer Flush" documentation section, and (4) merged dev into the branch.

1. Code Change Review

1.1 Core Logic Analysis

The core CouchbaseWriter.java production logic (constructor's registerFlushAction opt-in, doFlush(), close(), prepareCommit()) is byte-for-byte unchanged since my previous review at 50cec6aea1f. I re-confirmed this directly:

git diff 50cec6aea1f793429363a8151fae19e871cc47f2..e9a0bd5377d -- .../sink/CouchbaseWriter.java
# (empty output — no changes)

That means the engine FlushSignal dispatch-path analysis from my last review still holds without re-derivation — I traced SinkWriterContext.registerFlushAction/getFlushAction into SinkFlowLifeCycle.processSignal, called only from SinkFlowLifeCycle.received(Record<?>), the same entry point that dispatches processCheckpointBarrier/processDataRecord — so a FlushSignal really does arrive serialized on the task's own thread, with no concurrency against write/prepareCommit/close on Zeta. Runtime path is unchanged from before:

FakeSource rows → SinkFlowLifeCycle.received()processDataRecordCouchbaseWriter.write(row) buffers a WriteUnit (synchronized) → engine's periodic timer injects a FlushSignal at sink.flush.intervalprocessSignalwriterContext.getFlushAction().run()CouchbaseWriter.doFlush() (same monitor, same thread) → buffered rows upsert/insert-ed → buffer cleared.

What did change this round, scoped to the files touched between 50cec6aea1f and e9a0bd5377d:

Key Findings:

  1. Old Issue 1 (missing ASF license headers) — verified FIXED. Both CouchbaseWriterTest.java and fake_source_to_couchbase_timer_flush.conf now carry the standard header. Confirmed against the fork's actual CI run (KaustAbhinand/seatunnel run 33853034293): Run / License header is now success (previously the exact job that was failing and blocking everything downstream).
  2. Old Issue 2 (upsert/insert mock mismatch) — verified FIXED, independently re-traced. minimalOptions() now calls .withUpsertEnable(true) (CouchbaseWriterTest.java:65). I re-read all 5 tests line-by-line against this new default: doFlush() (CouchbaseWriter.java, unchanged) now correctly takes the collection.upsert(...) branch for every test, matching every stub/verify (when(collection.upsert(...)), doThrow(...).when(collection).upsert(...), verify(collection, times(1)).upsert(...)) in shouldRegisterFlushActionAndFlushBufferedRecordsOnSignal, shouldPropagateFlushFailure, shouldFlushOnCloseWhenEngineNeverInvokesFlushAction, shouldFlushOnPrepareCommitWhenEngineNeverInvokesFlushAction, and closeShouldKeepFlushExceptionWhenDisconnectAlsoThrows. This is a genuine fix, not a superficial one — the mismatch that would have failed all 5 tests deterministically is resolved.
  3. Old Issue 5 (stale docs) — verified FIXED. buffer-flush.interval was removed from the option table and example config in both docs/en/connectors/sink/Couchbase.md and docs/zh/connectors/sink/Couchbase.md; a new "Timer Flush" / "定时刷新" section was added to both, correctly documenting sink.flush.interval as a Zeta-only env-block engine primitive (I cross-checked the key name against EnvCommonOptions.java:89 and the equivalent Doris.md section — it's accurate). The now-dead buffer-flush.interval = 5000 line was also dropped from fake_source_to_couchbase.conf.
  4. Old Issue 3 (stale class Javadoc, CouchbaseWriter.java:58-59) — still open, unchanged. The class Javadoc still reads "A periodic background timer fires every buffer-flush.interval milliseconds..." — that config key no longer exists and there is no more background timer. See Issue 2 below (renumbered).
  5. Old Issue 4 (orphaned Javadoc, CouchbaseWriter.java:236-240) — still open, unchanged. The dangling /** Throws a {@link CouchbaseConnectorException} wrapping the latched async error... */ block, documenting nothing (its method was deleted before my first review), is still there. See Issue 4 below.
  6. New problem introduced by this round's own fix commits. The license-header commit (427964a5f19) left two blank lines between the header's closing */ and the package statement in CouchbaseWriterTest.java (lines 16-19), instead of one. This is exactly the kind of easy-to-miss whitespace slip that Spotless enforces — see Issue 1 below, this is currently the single thing blocking CI again, in the same masking pattern as the license-header issue did last round.
  7. Something I missed in my own previous review. CouchbaseSinkOptions.java (untouched by this round's commits, and also untouched between dev and my previously-reviewed commit 50cec6aea1f — i.e. it was already in this exact state when I did my first review) has a third instance of the same orphaned-Javadoc pattern I flagged in CouchbaseWriter.java, at lines 41-46, which I did not catch the first time. I apologize for the miss — see Issue 3 below.

1.2 Compatibility Impact

Unchanged from my previous assessment, and still fully compatible. The removed buffer-flush.interval option has never appeared in any released Apache SeaTunnel tag (git tag --contains ead62107e3 — the Couchbase sink's original addition — still returns nothing against the current tag list), so there is no production config this removal can break. No incompatible-changes.md entry is required.

1.3 Performance / Side-Effect Analysis

Unchanged from my previous assessment: net positive (fewer daemon threads, no awaitTermination(30s) per subtask on shutdown), no new side effects introduced by this round's changes (docs, test fixtures, and license headers only — no production code touched).

1.4 Error Handling and Logging

No production error-handling code changed this round. See Issues 1-4 below for the concrete problems in test/doc code found in this and the previous round.

Number Issue Location Problem Potential risk Best improvement Severity Status
1 Extra blank line breaks Spotless seatunnel-connectors-v2/connector-couchbase/src/test/java/.../sink/CouchbaseWriterTest.java:16-19 Two blank lines sit between the license header's closing */ and the package statement instead of one. Confirmed via the fork's actual CI run (KaustAbhinand/seatunnel run 33853034293, job Run / Code style, 100959933001): [ERROR] Failed to execute goal ... spotless-maven-plugin:2.29.0:check ... The following files had format violations: .../CouchbaseWriterTest.java. This is the only failing CI job right now, and — just like the License header failure last round — it blocks every downstream job (unit-test, all all-connectors-it-*, engine-v2-it, etc.) from running at all, meaning the correctness fix from Issue 2 of my last review (now verified correct by my own re-trace above) still has not actually been exercised by CI. Run ./mvnw spotless:apply -pl seatunnel-connectors-v2/connector-couchbase -am (or simply delete the extra blank line) and push. High New (introduced by this round's own license-header commit)
2 Stale class Javadoc seatunnel-connectors-v2/connector-couchbase/src/main/java/.../sink/CouchbaseWriter.java:58-59 Class Javadoc still documents the removed connector-owned timer and the removed buffer-flush.interval config key; the constructor's own inline comment 80 lines below correctly describes the new engine-driven mechanism, so the file now contradicts itself. A future maintainer opening this file for the first time reads a description of a mechanism (and a config key) that no longer exists in this class. Update the Javadoc bullet to describe the engine-level FlushSignal/registerFlushAction mechanism (Zeta) versus the size/checkpoint/close-only fallback (Spark/Flink), matching the constructor's comment. Medium Carryover, still unfixed
3 Orphaned Javadoc (missed by me previously) seatunnel-connectors-v2/connector-couchbase/src/main/java/.../config/CouchbaseSinkOptions.java:41-46 A dangling Javadoc block — "Maximum time (ms) between two consecutive batch writes. A value of -1 disables interval-based flushing." — documents the removed BUFFER_FLUSH_INTERVAL field. The field itself is already gone (removed in this PR's own a0ce9cbf132 commit, which predates and was already present at the commit I reviewed last time, 50cec6aea1f); only the comment was left behind, sitting directly above the unrelated RETRY_MAX Javadoc/field. Same class of dead/misleading documentation as Issue 4 below — a reader will look for a field this comment describes and not find one. Delete the orphaned comment block. Low-Medium Carryover — pre-existing at my first review, I missed it; apologies
4 Orphaned Javadoc seatunnel-connectors-v2/connector-couchbase/src/main/java/.../sink/CouchbaseWriter.java:236-240 Dangling Javadoc — "Throws a {@link CouchbaseConnectorException} wrapping the latched async error if one has been recorded by the background flush timer." — left over after checkAsyncFlushError() was deleted; sits between the "Internal helpers" section header and toJsonObject's own Javadoc. Same as Issue 3 — dead, misleading documentation. Delete the orphaned comment block. Low Carryover, still unfixed

2. Code Quality Assessment

2.1 Coding Standards

No new core methods or nontrivial fields were added by this round's commits (test fixtures, docs, and headers only). The three orphaned-Javadoc instances (Issues 2-4) remain the concrete gaps; nothing new on this front besides Issue 1's whitespace slip.

2.2 Test Coverage and Test Stability

Stable logic, but currently blocked from proving it. I re-verified the entire CouchbaseWriterTest.java suite line-by-line against the upsert-enable = true default now set in minimalOptions() and confirmed every stub/verify now matches the code path doFlush() actually takes — this is a real fix, not a superficial one, and I would expect all 5 tests to pass. However, CI cannot currently confirm this either way: the Run / Code style (Spotless) job fails first on the double-blank-line issue (Issue 1), and every downstream job including unit-test is skipped as a result — identical masking pattern to last round's License-header block. The pre-existing CouchbaseIT.testCouchbaseSinkTimerFlush E2E test is unchanged since my last review and remains well-constructed (correctly isolates the timer-flush path from checkpoint-triggered flush via a 5-minute checkpoint.interval vs 3-second sink.flush.interval).

Stability rating: Stable (both the unit test suite and the E2E test are logically correct as written), but unverified by CI — Issue 1 must be fixed and CI must actually turn green on unit-test before this can be called confirmed rather than just independently re-derived.

2.3 Documentation Updates

Verified fixed. Both docs/en/connectors/sink/Couchbase.md and docs/zh/connectors/sink/Couchbase.md now match the code: buffer-flush.interval is gone from the option table and example, and a new "Timer Flush" / "定时刷新" section documents sink.flush.interval consistently with the pattern established for Doris/ClickHouse/etc. One very minor nit, not rising to a blocking issue: the new hocon code fence in Couchbase.md (lines 177-181) is left unindented (sink.flush.interval = 10000 at column 0) while every other example block in the same file uses 4-space indentation inside braces — purely cosmetic, HOCON parses either way, feel free to leave it or tidy it up in a follow-up.

3. Architectural Soundness

3.1 Elegance of the Solution

Unchanged from my previous assessment: precise fix, reusing proven existing engine infrastructure (STIP-23) rather than inventing anything new. This round's remaining gaps (Issues 1-4) are all mechanical — a whitespace slip and three leftover doc comments — not design problems.

3.2 Maintainability

Improving, but not yet fully realized: the migration itself reduces connector-owned complexity as intended, but a maintainer still cannot look at a green CI run to confirm the new test suite actually passes (Issue 1), and three lingering orphaned-Javadoc blocks (Issues 2-4) will confuse the next person who opens these files.

3.3 Extensibility

Unchanged: neutral-to-positive, brings Couchbase in line with the registerFlushAction contract other sinks already implement.

3.4 Historical-Version Compatibility

Unchanged and reconfirmed: the Couchbase sink connector has never shipped in a released Apache SeaTunnel tag, so no incompatible-change documentation is required.

4. Issue Summary

# Issue Location Severity Status
1 Double blank line after license header breaks spotless:check, blocking all downstream CI (identical masking pattern to last round's License-header failure) CouchbaseWriterTest.java:16-19 High New this round
2 Class Javadoc still describes the removed background timer / buffer-flush.interval CouchbaseWriter.java:58-59 Medium Carryover, unfixed
3 Orphaned Javadoc documenting the removed BUFFER_FLUSH_INTERVAL field CouchbaseSinkOptions.java:41-46 Low-Medium Carryover — I missed this in my first review
4 Orphaned Javadoc left over from deleted checkAsyncFlushError() CouchbaseWriter.java:236-240 Low Carryover, unfixed

Resolved since last review (no longer open): missing ASF license headers (was High), upsert/insert test mock mismatch (was High), stale buffer-flush.interval documentation in docs/en/docs/zh (was Medium).

+1 to @nzw921rx's inline comment ("Please add license" on fake_source_to_couchbase_timer_flush.conf:18) — same issue as my old Issue 1, now resolved by this round's commits.

5. Merge Recommendation

Conclusion: Ready to merge after fixes

  1. Blockers — must be fixed:
    • Issue 1: fix the double blank line in CouchbaseWriterTest.java (run ./mvnw spotless:apply -pl seatunnel-connectors-v2/connector-couchbase -am) so CI can actually reach and pass unit-test — this is now the only thing standing between this PR and a fully green CI run.
  2. Recommended fixes — non-blocking:
    • Issue 2: update the CouchbaseWriter class Javadoc to describe the engine-level flush mechanism instead of the removed background timer.
    • Issue 3: delete the orphaned BUFFER_FLUSH_INTERVAL Javadoc block in CouchbaseSinkOptions.java.
    • Issue 4: delete the orphaned checkAsyncFlushError Javadoc block in CouchbaseWriter.java.

Overall assessment: great progress since the last round — you fixed both real blockers (license headers, and the trickier upsert/insert test mismatch, which I independently re-traced line-by-line rather than just taking on faith) and did a thorough docs pass that I hadn't even asked for in as much detail as you delivered (the new "Timer Flush" section is a genuinely good addition, not just a minimal patch). The one new problem is a one-line whitespace fix, and the remaining three items are all leftover doc comments, one of which I should have caught myself the first time around — sorry about that. None of this reflects on the core migration, which remains architecturally sound and unchanged since I last verified it. Once Issue 1 is fixed and CI goes green on unit-test, I don't see anything else standing in the way here. Thanks for sticking with this and for the careful work on the retry/dedup bookkeeping you preserved from the original implementation — nice work, and I'd be glad to take another look as soon as the blank line is fixed.

@KaustAbhinand

Copy link
Copy Markdown
Author

@DanielLeens, some of the unit tests fail due to the CouchbaseCloseTest - but with this migration, the entire file becomes deprecated, so the tests fail.

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

Thanks for the continued work, @KaustAbhinand — this is a full re-review of the latest head (2c58b8e176), not an incremental diff-only pass. I re-verified every issue from both of my previous rounds against the current code, and this time CI actually got far enough, for the first time, to run the connector-couchbase unit-test module and the connector-couchbase-e2e integration tests. That surfaced three deterministic, PR-caused failures — and two of them were already present at the very first commit I reviewed (50cec6aea1f), which I should have caught then and did not. I owe you an apology for that: in round 1 I even cited CouchbaseWriterCloseTest as supporting evidence for the close()/suppressed-exception behavior without checking whether that test still compiled cleanly against the code it was testing — it doesn't. Details and full carryover/new-this-round breakdown are below.

Good news first: all three items I flagged as fixed last round are genuinely fixed (double blank line, stale class Javadoc, orphaned checkAsyncFlushError Javadoc), and I independently re-verified each rather than trusting the commit message.

What Problem Does This PR Solve?

The Couchbase sink previously enforced its buffer's max-latency guarantee with a connector-owned ScheduledExecutorService background timer plus an AtomicReference-latched async-error mechanism so write/prepareCommit/close could detect and rethrow a failure raised on that background thread. This PR migrates CouchbaseWriter to the engine-level timer-flush mechanism (SinkWriter.Context#registerFlushAction, STIP-23) already adopted by ClickHouse, Doris, Elasticsearch, Hudi, JDBC, MongoDB, Prometheus, and StarRocks: on Zeta the engine now delivers a FlushSignal through the normal task-processing thread instead of a separate timer thread, removing the connector-owned scheduler, its shutdown/interrupt handling, and the async-error latch entirely, while keeping the size-based (buffer-flush.max-rows), checkpoint-based (prepareCommit), and close-based flush paths unchanged.

The value is a real simplification: roughly 40 lines of scheduler-lifecycle code and a whole async-error-latch mechanism collapse into a single context.registerFlushAction(this::doFlush) call in the constructor, removing one daemon thread per parallel sink subtask and the awaitTermination(30s) worst-case shutdown wait that went with it — at the cost of the max-latency guarantee now only existing on Zeta (Spark/Flink fall back to size/checkpoint/close-triggered flush only, which is documented and is the same trade-off every other STIP-23-migrated connector already made).

One-sentence summary: this replaces a connector-owned background timer thread and its bespoke error-propagation machinery with the engine's already-proven flush-signal mechanism, at no loss of behavior on Zeta and a documented, expected loss of sub-checkpoint latency guarantees on Spark/Flink (where the feature never worked cross-thread anyway in a way any released version depended on).

Before/after example — a Zeta job config that previously had no way to force a flush during an idle stream:

# Before: no max-latency guarantee possible; buffer only flushes on
# buffer-flush.max-rows, checkpoint, or close. A slow trickle of rows
# could sit unflushed indefinitely between checkpoints.
sink {
  Couchbase {
    connection.string = "couchbase://localhost"
    ...
    buffer-flush.max-rows = 1000
  }
}
# After: the engine flushes on a timer regardless of row volume (Zeta only).
env {
  sink.flush.interval = 10000
}
sink {
  Couchbase {
    connection.string = "couchbase://localhost"
    ...
    buffer-flush.max-rows = 1000
  }
}

1. Code Change Review

1.1 Core Logic Analysis

The production CouchbaseWriter.java flush/close/prepareCommit logic is unchanged since my first review at 50cec6aea1f (confirmed via git diff 50cec6aea1f..2c58b8e176 -- .../CouchbaseWriter.java, which only touches two Javadoc blocks — see below). The engine dispatch path I traced previously still holds: SinkWriterContext.registerFlushAction/getFlushAction (seatunnel-engine-server/.../task/context/SinkWriterContext.java:102) feeds SinkFlowLifeCycle.processSignal (.../task/flow/SinkFlowLifeCycle.java:728-736), called only from SinkFlowLifeCycle.received(Record<?>) — the same entry point that dispatches processCheckpointBarrier/processDataRecord. A FlushSignal therefore arrives serialized on the task's own thread, with no concurrency against write/prepareCommit/close on Zeta.

Fixed since my last review (verified independently, not just trusted):

  1. Class-level Javadoc (CouchbaseWriter.java, previously lines 58-59) no longer mentions the removed background timer or buffer-flush.interval.
  2. The orphaned checkAsyncFlushError Javadoc block (previously lines 236-240) is gone.
  3. The orphaned BUFFER_FLUSH_INTERVAL Javadoc in CouchbaseSinkOptions.java (previously lines 41-46) is gone.
  4. The double-blank-line Spotless violation in CouchbaseWriterTest.java is gone; the fork's own CI now passes both License header and Code style.

New problem introduced by this round's cleanup commit (Issue 4 below): the same edit that deleted the orphaned checkAsyncFlushError Javadoc also deleted the adjacent, still-valid Javadoc for toJsonObject() ("Converts a SeaTunnelRow to a JsonObject using the schema field names.") — toJsonObject (CouchbaseWriter.java:234) now has no comment at all. Low severity, but worth a one-line fix.

The two most important findings this round are both carryovers I missed in earlier rounds, now proven by CI rather than by static reading:

  1. A real E2E regression: the pre-existing testFakeSourceToCouchbaseSink test is now broken by this PR's own CouchbaseIT.startUp() changes. Before this PR, startUp() created a CREATE PRIMARY INDEX on COUCHBASE_COLLECTION (test_collection) and waited for it to go online. This PR's startUp() (lines 139-184) repurposes that entire block for the new COUCHBASE_COLLECTION_TIMER_FLUSH collection instead — the primary index for the original test_collection is never created anymore. testFakeSourceToCouchbaseSink (pre-existing, untouched by this PR) opens with cluster.query("DELETE FROM ... test_collection"), which requires that index. I confirmed this against the fork's actual CI: KaustAbhinand/seatunnel run 33856530525, job updated-modules-integration-test-part-1 (11, ubuntu-latest)testFakeSourceToCouchbaseSink fails deterministically (7/7 attempts) with com.couchbase.client.core.error.PlanningFailureException: ... No index available on keyspace default:test_bucket._default.test_collection ... Use CREATE PRIMARY INDEX ...; on the JDK 8 variant of the same job, all 14/14 CouchbaseIT test invocations fail. This same regression was already present at 50cec6aea1f, the commit I reviewed first — I should have caught this by diffing startUp() against dev at the time, and did not, because CI never got far enough (blocked by the license-header failure) for me to see it fail. See Issue 1 below.
  2. A real E2E design gap: testCouchbaseSinkTimerFlush is not restricted to the Zeta engine, but the feature it tests only exists on Zeta. Every other STIP-23 timer-flush E2E test in this codebase (DorisTimerFlushIT, ClickhouseTimerFlushIT, ElasticsearchTimerFlushIT, MongodbTimerFlushIT, PrometheusTimerFlushIT, StarRocksTimerFlushIT) carries a class-level @DisabledOnContainer(value = {}, type = {EngineType.SPARK, EngineType.FLINK}, disabledReason = "engine-level timer flush (sink.flush.interval) is only supported on Zeta engine"). CouchbaseIT has no such annotation anywhere, and testCouchbaseSinkTimerFlush is a plain @TestTemplate, so it also runs against the Flink and Spark TestContainers configured for this module. On those engines sink.flush.interval is a no-op (correctly documented as such in this PR's own new "Timer Flush" doc section), checkpoint.interval = 300000 in the test's conf never fires within the test window, and buffer-flush.max-rows = 100000 is never reached by the 10 fake rows — so the sink never flushes and the count-based assertion can never be satisfied on Flink/Spark. I confirmed this directly in the fork's CI logs (same job as above): on the TestContainer[Flink:1.13.6] iteration, testCouchbaseSinkTimerFlush fails/times out repeatedly with org.awaitility.core.ConditionTimeoutException: ... The streaming job must still be running when timer flush publishes the buffered rows ==> expected: <false> but was: <true> within 1 minutes. This test was already present at 50cec6aea1f in this same unrestricted form. In my first review I called this test "well-constructed" based on reading the Zeta-path logic alone — I did not check it against the sibling connectors' established @DisabledOnContainer convention, and CI had not yet reached it. That was a miss on my part; I'm flagging it now as a carryover, not a new-version issue. See Issue 2 below.
  3. CouchbaseWriterCloseTest.java (pre-existing file, not touched by this PR beyond removing the now-nonexistent .withBatchIntervalMs(-1) call) is broken by this same migration and was never updated or removed. It reflectively reads a field asyncFlushError (CouchbaseWriter.class.getDeclaredField("asyncFlushError")) that this PR's underlying migration deleted entirely — that field was already gone at 50cec6aea1f. 3 of its 4 tests fail deterministically with NoSuchFieldException, confirmed in the fork's own unit-test CI job across all four OS/JDK matrix combinations. You flagged this yourself in your 09:49 comment ("the entire file becomes deprecated, so the tests fail") — that's the correct diagnosis, but the fix (deleting or rewriting the file) hasn't landed yet. I should have caught this in round 1 too: I cited "the existing CouchbaseWriterCloseTest" as corroborating evidence for the close()/suppressed-exception behavior without verifying the test still compiled cleanly against the field it reflects into. See Issue 3 below.

Runtime path (Zeta, streaming job with sink.flush.interval set in env) — unchanged since my last review: FakeSource rows → SinkFlowLifeCycle.received()processDataRecordCouchbaseWriter.write(row) buffers a WriteUnit (synchronized) → the engine's periodic timer injects a FlushSignal at sink.flush.intervalprocessSignalwriterContext.getFlushAction().run()CouchbaseWriter.doFlush() (same monitor, same thread) → buffered rows upsert/insert-ed → buffer cleared. On checkpoint, prepareCommit() still drives doFlush() synchronously; on close, a final doFlush() still runs before cluster.disconnect(). None of this changed this round.

1.2 Compatibility Impact

Fully compatible. Unchanged conclusion from my previous rounds, reconfirmed: the removed buffer-flush.interval option has never appeared in a released Apache SeaTunnel tag (git tag --contains ead62107e3 — the Couchbase sink's original addition — returns nothing against the current tag list, most recent tag 2.3.13). No production config in the wild depends on this option, so its removal is not an incompatible change in the sense this project's compatibility rules protect against, and no incompatible-changes.md entry is required.

1.3 Performance / Side-Effect Analysis

Unchanged and still net positive: one fewer daemon thread per parallel sink subtask, no more per-subtask awaitTermination(30s) on shutdown. No production code changed this round (only test/doc/Javadoc edits), so there is nothing new to assess here.

1.4 Error Handling and Logging

No production error-handling code changed this round. The concrete problems found this round are all in test code (Issues 1-3) or a missing comment (Issue 4); see the table below.

Number Issue Location Problem Potential risk Best improvement Severity Status
1 CouchbaseIT.java startUp(), lines 139-184 Primary-index creation was retargeted from COUCHBASE_COLLECTION (test_collection) to the new COUCHBASE_COLLECTION_TIMER_FLUSH collection instead of being added alongside it. The original collection's primary index is never created. Breaks the pre-existing, unrelated testFakeSourceToCouchbaseSink test: its DELETE FROM ... test_collection N1QL statement fails with PlanningFailureException: No index available. Confirmed in fork CI: 7/7 (JDK 11) and 14/14 (JDK 8) CouchbaseIT test invocations fail in updated-modules-integration-test-part-1. Keep both index-creation/wait blocks: re-add the original CREATE PRIMARY INDEX + status-poll for COUCHBASE_COLLECTION, and keep the new one for COUCHBASE_COLLECTION_TIMER_FLUSH alongside it. High Carryover from round 1 — I missed it then; confirmed broken by CI this round
2 CouchbaseIT.java, testCouchbaseSinkTimerFlush (line 301) and the class declaration (line 57) The new timer-flush E2E test has no @DisabledOnContainer(type = {EngineType.SPARK, EngineType.FLINK}, ...) restriction, unlike every other STIP-23 connector's timer-flush E2E test in this codebase (Doris, ClickHouse, Elasticsearch, MongoDB, Prometheus, StarRocks all carry this exact annotation). sink.flush.interval is a Zeta-only no-op on Spark/Flink, so the test's premise cannot hold there. Test fails/times out on the Flink (and, by the same logic, Spark) TestContainer iterations. Confirmed in fork CI: testCouchbaseSinkTimerFlush{TestContainer} on Flink:1.13.6 fails with ConditionTimeoutException ("streaming job must still be running..."). Either move testCouchbaseSinkTimerFlush into a separate class annotated @DisabledOnContainer(value = {}, type = {EngineType.SPARK, EngineType.FLINK}, disabledReason = "engine-level timer flush (sink.flush.interval) is only supported on Zeta engine") (matching the sibling-connector pattern exactly), or add the same annotation to the existing class if no other test in it needs Spark/Flink coverage. High Carryover from round 1 — I called this test "well-constructed" without checking the sibling @DisabledOnContainer convention; confirmed broken by CI this round
3 seatunnel-connectors-v2/connector-couchbase/src/test/java/.../sink/CouchbaseWriterCloseTest.java (whole file, pre-existing) Reflectively accesses CouchbaseWriter's asyncFlushError field (injectAsyncFlushError, line 108), which this migration removed entirely. 3 of 4 @Test methods fail with NoSuchFieldException. Confirmed in fork CI unit-test job on all 4 matrix combinations (JDK 8/11 × ubuntu/windows): Tests run: 4, Failures: 0, Errors: 3. You already correctly diagnosed this yourself in your 09:49 comment. Deterministic CI failure on every run until fixed; blocks the reactor build for connector-couchbase (the module shows FAILURE, and connector-couchbase-e2e is then SKIPPED in that job). Delete CouchbaseWriterCloseTest.java — its premise (a background-timer-latched async error) no longer exists, and the one behavior in it that's still meaningful (close() disconnects the cluster even when flush fails, with disconnect failure suppressed rather than swallowed) is already covered by the new CouchbaseWriterTest.closeShouldKeepFlushExceptionWhenDisconnectAlsoThrows. If you'd rather keep some of it, only close_withNoLatchedError_disconnectsCluster doesn't touch the removed field and could be ported into CouchbaseWriterTest, but I'd just delete the whole file. High Carryover from round 1 — I cited this test as valid supporting evidence without checking it against the already-removed field; confirmed broken by CI this round
4 CouchbaseWriter.java, around line 234 This round's cleanup commit, while correctly deleting the orphaned checkAsyncFlushError Javadoc, also deleted the adjacent, still-valid Javadoc for toJsonObject() ("Converts a SeaTunnelRow to a JsonObject using the schema field names."). toJsonObject now has no comment. Minor: a genuinely useful one-line comment on a real, still-used private helper was lost as collateral damage of an unrelated cleanup. Re-add the one-line Javadoc for toJsonObject. Low New this round (introduced by the same commit that fixed Issue 4 from last round)

2. Code Quality Assessment

2.1 Coding Standards

The production code itself remains clean and well-commented (unchanged this round). The gaps are all in test files: two pre-existing test files (CouchbaseWriterCloseTest.java, and by extension the E2E CouchbaseIT.java) that were not updated to match the migration they sit next to, plus one small comment regression (Issue 4).

2.2 Test Coverage and Test Stability

High risk — this is a formal rating, and it is CI-confirmed rather than inferred. The good news: the new CouchbaseWriterTest.java (5 tests covering signal-driven flush, close-fallback flush, checkpoint flush, and suppressed-exception-on-close) passes cleanly in the fork's CI (Tests run: 5, Failures: 0, Errors: 0) — the upsert/insert mismatch from round 1 is genuinely fixed and I re-confirmed it stayed fixed. The bad news is everything else: CouchbaseWriterCloseTest.java fails deterministically (Issue 3), and the connector-couchbase-e2e module fails deterministically and near-totally (13/14 to 14/14 test invocations across the matrix) due to Issues 1 and 2. None of these three failures are timing-sensitive flakes — they reproduce on every JDK/OS combination in the matrix, and the root causes (a missing index, a missing engine-restriction annotation, a stale reflective field reference) are all structural, not environmental. This is unrelated to the previously-known Couchbase-container-startup flake (Connection reset during containerIsStarting, ~19s, fixed by PR #11845) — I checked, and none of these failures show that signature; the container starts and runs tests successfully, the tests themselves then fail on their own assertions/reflection.

2.3 Documentation Updates

Still good, unchanged since last round: docs/en/connectors/sink/Couchbase.md and docs/zh/connectors/sink/Couchbase.md correctly drop buffer-flush.interval and add a "Timer Flush" / "定时刷新" section that correctly states the Zeta-only scope of sink.flush.interval — which, notably, is the exact scoping fact that Issue 2 shows the E2E test itself doesn't respect. One very minor, non-blocking nit: the Chinese doc's new section omits the English version's explanatory paragraph about the Spark/Flink fallback path (size/checkpoint/close-only flush) — purely a completeness/consistency nit, not incorrect.

3. Architectural Soundness

3.1 Elegance of the Solution

Unchanged from my previous assessment: this is a precise, minimal application of proven, already-adopted engine infrastructure (STIP-23) rather than new invention. The approach itself is sound; every open issue this round is either test/doc cleanup debt or a genuine E2E test-authoring gap, not a flaw in the underlying design.

3.2 Maintainability

Currently undermined, more than I gave credit for in previous rounds: two E2E regressions and one broken unit-test file mean a maintainer cannot currently trust a green connector-couchbase or connector-couchbase-e2e CI run to mean what it should. Once Issues 1-3 are fixed, maintainability should be net-improved by the migration itself (less connector-owned thread-lifecycle code).

3.3 Extensibility

Unchanged: neutral-to-positive — this brings Couchbase in line with the registerFlushAction contract other sinks already implement.

3.4 Historical-Version Compatibility

Unchanged and reconfirmed: the Couchbase sink connector has never shipped in a released Apache SeaTunnel tag, so removing buffer-flush.interval requires no incompatible-changes.md entry or migration guidance.

Registration-chain audit (connector migration checklist): this PR does not touch connector discovery at all — CouchbaseSinkFactory still carries @AutoService(Factory.class) unmodified, plugin-mapping.properties still maps seatunnel.sink.Couchbase = connector-couchbase unmodified, the module is still listed in seatunnel-connectors-v2/pom.xml and referenced from seatunnel-dist/pom.xml unmodified, and no META-INF/services files are checked into source (this connector relies on @AutoService code generation, consistent with the rest of connectors-v2). I found no broken link in the registration chain. Separately, CouchbaseSink does not implement SupportMultiTableSinkWriter (confirmed via grep, no matches in the module), so the associated three-point multi-table-sink checklist does not apply here.

4. Issue Summary

# Issue Location Severity Status
1 startUp() creates a primary index only for the new timer-flush collection, silently dropping it for the original collection — breaks the pre-existing testFakeSourceToCouchbaseSink E2E test CouchbaseIT.java:139-184 High Carryover from round 1, confirmed broken by CI this round
2 New testCouchbaseSinkTimerFlush E2E test lacks the @DisabledOnContainer(SPARK, FLINK) restriction every sibling STIP-23 timer-flush E2E test uses, so it fails/hangs on Flink and Spark where the feature is a no-op CouchbaseIT.java:57,301 High Carryover from round 1, confirmed broken by CI this round
3 CouchbaseWriterCloseTest.java reflectively reads the now-deleted asyncFlushError field; 3/4 tests fail with NoSuchFieldException on every CI run CouchbaseWriterCloseTest.java (whole file) High Carryover from round 1, confirmed broken by CI this round
4 The still-valid toJsonObject() Javadoc was accidentally deleted alongside the orphaned Javadoc it sat next to CouchbaseWriter.java:~234 Low New this round

Resolved since last review (no longer open): double-blank-line Spotless violation, stale class Javadoc describing the removed timer, orphaned checkAsyncFlushError Javadoc, orphaned BUFFER_FLUSH_INTERVAL Javadoc.

5. Merge Recommendation

Conclusion: Not recommended for merge

  1. Blockers — must be fixed (sorted by severity, all three are High and all three are CI-confirmed deterministic failures, not flakes):
    • Issue 1: restore primary-index creation (+ readiness wait) for the original test_collection, alongside the new one for test_collection_timer_flush, in CouchbaseIT.startUp().
    • Issue 2: restrict testCouchbaseSinkTimerFlush to the Zeta engine only, using the same @DisabledOnContainer(value = {}, type = {EngineType.SPARK, EngineType.FLINK}, disabledReason = "engine-level timer flush (sink.flush.interval) is only supported on Zeta engine") pattern already used by DorisTimerFlushIT/ClickhouseTimerFlushIT/etc.
    • Issue 3: delete (or, at minimum, fix) CouchbaseWriterCloseTest.java — it tests a mechanism this same PR removes, and currently fails deterministically in CI.
  2. Recommended fixes — non-blocking:
    • Issue 4: restore the one-line Javadoc for toJsonObject() that was accidentally deleted this round.

Overall assessment: the core migration (CouchbaseWriter's move to registerFlushAction) remains architecturally sound, and this round's own cleanup work (fixing the Spotless violation and the two remaining orphaned-Javadoc blocks) is genuinely done correctly. But this is the first round where CI actually ran far enough to execute the connector-couchbase unit tests and the connector-couchbase-e2e integration tests, and it shows the migration is not yet complete: two real E2E regressions and one broken pre-existing unit-test file, all three tracing back to test infrastructure that should have been updated alongside the production code but wasn't. I want to be upfront that two of these three issues were discoverable by careful static reading back in round 1, and I missed them — I'm sorry for the extra round-trip that costs you. None of this reflects on the soundness of the underlying registerFlushAction approach, which is unchanged and correct. These are all mechanical, well-scoped fixes (restore one index-creation block, add one annotation, delete one obsolete test file) rather than design changes, and I'd be glad to take another look as soon as they're in — thank you for sticking with this through three rounds, the core migration work has been solid throughout.

@KaustAbhinand

Copy link
Copy Markdown
Author

@DanielLeens - The thing is, with the migration, CouchbaseCloseTest is no longer needed, but I don't think I can delete it, (I had mentioned that some of the unit tests would fail due to this in a comment above)

I'll fix the other 2 issues and re-commit

@KaustAbhinand

Copy link
Copy Markdown
Author

@DanielLeens - The primary index creation was already there in CouchbaseIT, I think you may have missed it.
I have added the disabledOnContainer to CouchbaseWriterTest now.

And I have deleted the CouchbaseCloseTest file.

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

Thanks for the fast turnaround, @KaustAbhinand, and for the two follow-up comments. This is a full re-review of the current head (965caa425c), not an incremental diff-only pass — I re-read the entire cumulative diff against dev and re-verified every prior finding against fresh CI evidence from the fork's own run for this exact commit (KaustAbhinand/seatunnel run 33869682781, completed 2026-09-04T13:20:10Z). Two of the three prior blockers are genuinely fixed and CI-confirmed. The third is not, and I need to correct the record on your 11:52 comment: you said "the primary index creation was already there in CouchbaseIT, I think you may have missed it" — I checked the diff between the commit I last reviewed (2c58b8e176) and this one, and CouchbaseIT.startUp() was not touched at all in that commit; only the CouchbaseWriterCloseTest.java deletion and the @DisabledOnContainer annotation landed. The primary-index gap for test_collection is unchanged from last round, and the fork's own CI for this exact head shows testFakeSourceToCouchbaseSink still failing deterministically (7/7) with the same PlanningFailureException: No index available on keyspace ... test_collection. Also, to clear up the naming in your comment: the @DisabledOnContainer annotation was correctly added to CouchbaseIT.testCouchbaseSinkTimerFlush (not CouchbaseWriterTest, which is a plain unit test class with no engine-container concept) — the fix itself is correct, just a naming slip in your comment, not in the code.

What Problem Does This PR Solve?

The Couchbase sink previously enforced its buffer's max-latency guarantee with a connector-owned ScheduledExecutorService background timer plus an AtomicReference-latched async-error mechanism so write/prepareCommit/close could detect and rethrow a failure raised on that background thread. This PR migrates CouchbaseWriter to the engine-level timer-flush mechanism (SinkWriter.Context#registerFlushAction, STIP-23) already adopted by ClickHouse, Doris, Elasticsearch, Hudi, JDBC, MongoDB, Prometheus, and StarRocks: on Zeta the engine now delivers a FlushSignal through the normal task-processing thread instead of a separate timer thread, removing the connector-owned scheduler, its shutdown/interrupt handling, and the async-error latch entirely, while keeping the size-based (buffer-flush.max-rows), checkpoint-based (prepareCommit), and close-based flush paths unchanged.

The value is a real simplification: roughly 40 lines of scheduler-lifecycle code and a whole async-error-latch mechanism collapse into a single context.registerFlushAction(this::doFlush) call in the constructor, removing one daemon thread per parallel sink subtask and the awaitTermination(30s) worst-case shutdown wait that went with it — at the cost of the max-latency guarantee now only existing on Zeta (Spark/Flink fall back to size/checkpoint/close-triggered flush only, which is documented and is the same trade-off every other STIP-23-migrated connector already made).

One-sentence summary: this replaces a connector-owned background timer thread and its bespoke error-propagation machinery with the engine's already-proven flush-signal mechanism, at no loss of behavior on Zeta and a documented, expected loss of sub-checkpoint latency guarantees on Spark/Flink.

Simple example — a Zeta streaming job config that previously had no way to force a flush during an idle stream:

env {
  sink.flush.interval = 10000   # engine-level timer, Zeta only
}
sink {
  Couchbase {
    connection.string = "couchbase://localhost"
    bucket = "my_bucket"
    collection = "my_collection"
    buffer-flush.max-rows = 1000
  }
}

1. Code Change Review

1.1 Core Logic Analysis

Production CouchbaseWriter.java logic is unchanged since my first review at 50cec6aea1f — confirmed via git diff 50cec6aea1f..965caa425c -- .../CouchbaseWriter.java, which only touches Javadoc blocks and the timer-lifecycle removal itself, no new production-code edits this round. Runtime dispatch chain, retraced end to end for this re-review: SinkWriterContext.registerFlushAction/getFlushAction (seatunnel-engine-server/.../task/context/SinkWriterContext.java:102) feeds SinkFlowLifeCycle.processSignal (.../task/flow/SinkFlowLifeCycle.java:728-736), invoked only from SinkFlowLifeCycle.received(Record<?>) — the same entry point that dispatches processCheckpointBarrier/processDataRecord. A FlushSignal therefore arrives serialized on the task's own thread, with no concurrency against write/prepareCommit/close on Zeta. On Spark/Flink, context.registerFlushAction(...) hits the interface's no-op default (the context types there do not implement it), so the buffer is flushed only on buffer-flush.max-rows, prepareCommit() (checkpoint), and close() — this is documented correctly in the new "Timer Flush" doc section.

Verified status of the three prior blockers against this exact head, using fresh CI evidence rather than static reading alone:

  1. Issue 1 (primary index for test_collection) — NOT FIXED. CouchbaseIT.startUp() (lines 127-190 in the current file) still only creates a CREATE COLLECTION for the original test_collection (line 127-139) but the CREATE PRIMARY INDEX + online-status-poll block was retargeted entirely to the new test_collection_timer_flush collection in an earlier round and is still that way now (lines 156-189) — there is no primary-index creation for test_collection anywhere in the file. The incremental diff between 2c58b8e176 and 965caa425c (git diff 2c58b8e176..965caa425c -- .../CouchbaseIT.java) touches only the @DisabledOnContainer addition, confirming startUp() was not revisited this round. Fresh CI evidence for this exact head: fork run 33869682781, job updated-modules-integration-test-part-1 (11, ubuntu-latest) (https://github.com/KaustAbhinand/seatunnel/actions/runs/33869682781/job/101015786261) — testFakeSourceToCouchbaseSink fails 7/7 times with PlanningFailureException: ... No index available on keyspace default:test_bucket._default.test_collection ...; the JDK 8 job shows the identical pattern. [ERROR] Tests run: 8, Failures: 0, Errors: 7 for CouchbaseIT in that job — the only passing test is the new testCouchbaseSinkTimerFlush.
  2. Issue 2 (@DisabledOnContainer on the timer-flush E2E test) — FIXED, CI-confirmed. testCouchbaseSinkTimerFlush (line 302) now carries @DisabledOnContainer(value = {}, type = {EngineType.SPARK, EngineType.FLINK}, disabledReason = "engine-level timer flush (sink.flush.interval) is only supported on Zeta engine"), matching the sibling STIP-23 connectors' pattern exactly. Fork CI for this head shows testCouchbaseSinkTimerFlush ... successfully run (job log line ~20942), no longer attempted on the Flink/Spark TestContainer iterations.
  3. Issue 3 (CouchbaseWriterCloseTest.java reflecting into the deleted asyncFlushError field) — FIXED, CI-confirmed. The file is deleted (confirmed via gh pr diff showing deleted file mode 100644, and it is absent in the current tree). All four unit-test matrix jobs (JDK 8/11 × ubuntu/windows) now report conclusion: success for this head, versus 3/4 test errors previously.
  4. Issue 4 (missing toJsonObject() Javadoc, Low/non-blocking from last round) — still not restored. CouchbaseWriter.java:234 (private JsonObject toJsonObject(SeaTunnelRow row)) still has no comment above it in the current head. Not a blocker, just noting it remains open.

No new issues were introduced by this round's commit. The incremental diff is exactly two changes: delete CouchbaseWriterCloseTest.java, add the @DisabledOnContainer annotation. Both are correct and minimal.

1.2 Compatibility Impact

Fully compatible. Unchanged conclusion, reconfirmed this round: buffer-flush.interval has never appeared in a released Apache SeaTunnel tag (most recent tag 2.3.13 predates this connector's buffer-flush.interval option entirely). No production config depends on the removed option, so no incompatible-changes.md entry is required.

1.3 Performance / Side-Effect Analysis

Unchanged and still net positive: one fewer daemon thread per parallel sink subtask on Zeta, no more per-subtask awaitTermination(30s) on shutdown. No production code changed this round (test-only changes), so there is nothing new to assess.

1.4 Error Handling and Logging

No production error-handling code changed this round. doFlush()'s exception path in close() (suppressed-disconnect-exception handling) is unchanged and still correctly covered by CouchbaseWriterTest.closeShouldKeepFlushExceptionWhenDisconnectAlsoThrows.

2. Code Quality Assessment

2.1 Coding Standards

Production code remains clean. The one remaining gap is the missing toJsonObject() Javadoc (Issue 4, carried over, Low severity). License headers and Spotless formatting are clean per the fork's own License header/Code style checks.

2.2 Test Coverage and Test Stability

High risk — CI-confirmed, not inferred. CouchbaseWriterTest.java (5 tests: signal-driven flush, close-fallback flush, checkpoint flush, flush-failure propagation, suppressed-disconnect-exception) passes cleanly across all 4 unit-test matrix jobs. CouchbaseWriterCloseTest.java no longer exists, so its prior deterministic failures are gone. However, connector-couchbase-e2e still fails deterministically on every CI run because of Issue 1: 7 of 8 CouchbaseIT test invocations error in updated-modules-integration-test-part-1 on both JDK 8 and JDK 11. This is not a flake — it reproduces every time, on every engine variant (TestContainer[Zeta], Flink:1.13/1.14/1.15, Spark:2.4/3.3), because the root cause (missing primary index) is structural, not environmental. It is unrelated to the known Couchbase-container-startup flake (Connection reset during containerIsStarting, fixed by #11845) — the container starts fine; the DELETE statement itself fails at query-planning time.

2.3 Documentation Updates

Unchanged from last round: docs/en/connectors/sink/Couchbase.md and docs/zh/connectors/sink/Couchbase.md correctly drop buffer-flush.interval and add a "Timer Flush" / "定时刷新" section that correctly states the Zeta-only scope. One still-open, non-blocking nit: the Chinese doc's new section omits the English version's explanatory paragraph about the Spark/Flink fallback path (size/checkpoint/close-only flush) — a completeness/consistency nit, not incorrect.

3. Architectural Soundness

3.1 Elegance of the Solution

Unchanged: a precise, minimal application of proven, already-adopted engine infrastructure (STIP-23) rather than new invention. Every open issue is test infrastructure debt, not a design flaw.

3.2 Maintainability

Still undermined by Issue 1: as long as testFakeSourceToCouchbaseSink fails deterministically, a maintainer cannot trust a green connector-couchbase-e2e run to mean what it should, and the updated-modules-integration-test-part-1 job stays red on every push touching this module or nearby ones. Once Issue 1 is fixed, maintainability is net-improved by the migration itself (less connector-owned thread-lifecycle code, one fewer test file to maintain).

3.3 Extensibility

Unchanged: neutral-to-positive — brings Couchbase in line with the registerFlushAction contract other sinks already implement.

3.4 Historical-Version Compatibility

Unchanged and reconfirmed: the Couchbase sink connector has never shipped in a released Apache SeaTunnel tag, so removing buffer-flush.interval requires no incompatible-changes.md entry or migration guidance.

Registration-chain audit (connector migration checklist): this PR does not touch connector discovery. CouchbaseSinkFactory still carries @AutoService(Factory.class) unmodified, plugin-mapping.properties still maps seatunnel.sink.Couchbase = connector-couchbase unmodified, the module is still listed in seatunnel-connectors-v2/pom.xml / seatunnel-dist/pom.xml unmodified, and CouchbaseSink does not implement SupportMultiTableSinkWriter. No broken link in the registration chain.

4. Issue Summary

# Issue Location Severity Status
1 startUp() never creates a primary index for the original test_collection (it was retargeted to the new timer-flush collection in an earlier round); breaks the pre-existing testFakeSourceToCouchbaseSink test CouchbaseIT.java:127-190 High Still open — carryover, not touched by this round's commit, CI-confirmed failing (7/8 errors) on the current head
2 New testCouchbaseSinkTimerFlush E2E test lacked the Spark/Flink @DisabledOnContainer restriction CouchbaseIT.java:302-307 High Fixed and CI-confirmed passing this round
3 CouchbaseWriterCloseTest.java reflectively read the deleted asyncFlushError field (deleted file) High Fixed — file deleted, all unit-test matrix jobs now green
4 toJsonObject() Javadoc was accidentally deleted alongside an adjacent orphaned Javadoc block CouchbaseWriter.java:234 Low Still open, non-blocking

5. Merge Recommendation

Conclusion: Not recommended for merge

Blockers — must be fixed:

  • Issue 1: restore primary-index creation (+ readiness wait) for the original test_collection in CouchbaseIT.startUp(), alongside the existing block for test_collection_timer_flush. This is the only remaining blocker; it is a self-contained, mechanical fix (add back the CREATE PRIMARY INDEX ON ... test_collection + online-status poll that this migration's earlier round removed) and does not require touching any production code.

Recommended fixes — non-blocking:

  • Issue 4: restore the one-line Javadoc for toJsonObject() that was accidentally deleted two rounds ago.
  • Documentation: add the Spark/Flink-fallback explanatory paragraph to the Chinese "定时刷新" section to match the English "Timer Flush" section.

Overall assessment: this round's commit did exactly what it said it would do — deleted the obsolete unit test and added the missing engine restriction — and both changes are correct and fully CI-verified on the exact head being reviewed. The underlying registerFlushAction migration remains architecturally sound and unchanged. The one remaining blocker (Issue 1) is a narrow, well-understood, single-method fix with a precise CI failure signature to validate against; once it lands and the fork's updated-modules-integration-test-part-1 job goes green for both JDK 8 and JDK 11, this should be ready to merge on the next pass.

@KaustAbhinand

KaustAbhinand commented Sep 4, 2026

Copy link
Copy Markdown
Author

Hi @DanielLeens! The CI is finally green, can it be merged now? All the unit tests have passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature][Connector-V2][Couchbase] Migrate CouchbaseWriter to engine-level timer flush

4 participants