Repository navigation
IGNITE-19661 In-memory CDC should not produce WAL records when disabled. - #13649
NSAmelchev wants to merge 1 commit into
Conversation
1bc1e97 to
420a0f2
Compare
420a0f2 to
a5cc7a8
Compare
nizhikov
left a comment
nizhikov
left a comment
There was a problem hiding this comment.
Review of the rebased version (a5cc7a8). The record-free design is a clear simplification over the previous revisions.
Severity legend: [Blocker] = data loss or wrong behaviour, should be fixed before merge. [Should fix] = hot-path cost or compatibility break. [Minor] = cleanup, no behaviour change.
Summary:
- 2 blockers in
CdcLinkManager(no gap after restart while disabled; lagging archiver drops pre-disable segments). - 2 should-fix (per-record check in
log(); abstract method on the plugin-facing WAL interface). - 3 minor cleanups.
Details inline.
| /** */ | ||
| private void markDisableSegment() { | ||
| // -1 if the WAL logging is not resumed yet: the node is restarted with CDC disabled, the gap is already made. | ||
| disableSgmnt = Math.max(disableSgmnt, cctx.wal(true).currentSegment()); |
There was a problem hiding this comment.
[Blocker] markDisableSegment() is called from the property listener, which fires at node start (onReadyForRead) before logging is resumed, so currentSegment() returns -1 and nothing is marked. The comment says "the gap is already made", but that does not hold for an in-memory-only node: CDC disabled at segment N, nothing is logged afterwards, node restarted. resumeLogging continues in N, disableSgmnt stays -1, CDC is re-enabled, records go into N, and N is archived and linked right after N-1. The consumer sees consecutive segments and every change made while disabled is silently lost. Same for a persistent node with an in-memory CDC region that restarts or joins while the property is already true.
Suggest re-running markDisableSegment() from resumeLogging (or after restoreWriteHandle) when forceDisabled() && inMemoryCdcRegion.
| void linkSegment(long idx, File segment) throws IOException { | ||
| if (forceDisabled()) | ||
| log.warning("Creation of segment CDC link skipped. '" + CDC_DISABLED + "' distributed property is 'true'."); | ||
| else if (idx <= disableSgmnt) |
There was a problem hiding this comment.
[Blocker] idx <= disableSgmnt also drops segments older than the disable segment that were still waiting in the WAL work dir for the archiver, even though they hold changes made while CDC was enabled. Example: archiver lags, N-3..N-1 are not archived yet, CDC is disabled at N and quickly re-enabled. The old code linked N-3..N-1 once archived; this code refuses them with "was not archived before CDC was disabled". Up to walSegments - 1 segments of pre-disable data are lost beyond the one documented skipped segment.
Only idx == disableSgmnt is needed for the gap; forceDisabled() above already covers the still-disabled case.
| if (currWrHandle == null || (isDisable != null && isDisable.check())) | ||
| return null; | ||
|
|
||
| if (cdcLinkMgr != null && cdcLinkMgr.shouldRollover(currWrHandle.getSegmentId())) { |
There was a problem hiding this comment.
[Should fix] This adds a volatile read and a branch to every log() call on every CDC-configured node, including persistent nodes where CDC is never disabled. The gap also only materialises when a later record arrives: on an in-memory-only node nothing is logged while CDC is disabled, so the disable segment stays open until CDC is re-enabled and something is written, and the archive / CDC dir does not reflect the disable until then.
Rolling the disable segment over asynchronously from the listener (the same way TimeoutRollover / closeBufAndRollover do it) keeps log() untouched and makes the gap independent of future writes.
| /** | ||
| * @return {@code True} if CDC is disabled by the {@code cdc.disabled} distributed property. | ||
| */ | ||
| public boolean cdcForceDisabled(); |
There was a problem hiding this comment.
[Should fix] A new abstract method on a plugin-facing interface. GridCacheProcessor obtains the WAL manager via ctx.plugins().createComponent(IgniteWriteAheadLogManager.class), so any plugin implementation that does not extend FileWriteAheadLogManager stops compiling.
default boolean cdcForceDisabled() { return false; } keeps compatibility and also removes the NoOpWALManager edit.
| } | ||
|
|
||
| /** */ | ||
| private void markDisableSegment() { |
There was a problem hiding this comment.
[Minor] CdcLinkManager is created by FileWriteAheadLogManager but reaches back to its owner through cctx.wal(true) (with its assert) just to read currentSegment(). Correctness here depends on cctx.wal(true) resolving to the same instance that owns this manager. Passing the owning manager or a LongSupplier for the current segment into the constructor removes the hidden dependency and makes it testable.
| */ | ||
| public boolean logDataRecords() { | ||
| return walEnabled() && (persistenceEnabled || cdcEnabled()); | ||
| return walEnabled() && (persistenceEnabled || (cdcEnabled() && !wal().cdcForceDisabled())); |
There was a problem hiding this comment.
[Minor] wal() is ctx.wal(cdcEnabled()), so dataRegion.config().isCdcEnabled() is now evaluated twice on this per-entry path. The branch is only reached when cdcEnabled() is already true, so ctx.wal(true).cdcForceDisabled() is enough.
|
|
||
| /** */ | ||
| private static List<Long> linkedSegments(NodeFileTree ft) { | ||
| return Arrays.stream(ft.walCdcSegments()).map(File::toPath).map(ft::walSegmentIndex).sorted() |
There was a problem hiding this comment.
[Minor] This "linked CDC segment indices" computation is now copy-pasted in three test classes (CdcSelfTest.linkedSegments, WalForCdcTest, CdcCommandTest.checkLinks) with small variations (sorted vs unsorted, List vs stream). One helper on NodeFileTree (e.g. walCdcSegmentIndexes()) or in AbstractCdcTest would cover all three.
2 participants
Caches in data regions without persistence stop logging data records while CDC is disabled by the
cdc.disableddistributed property. To keep the loss visible, on nodes with such regions the segment that is current at the disabling is closed by the next record and never linked to the CDC directory, soignite-cdc.shfails with "Found missed segments" after CDC is enabled again, even if no segment was archived while it was disabled. The link logic is moved fromFileWriteAheadLogManagerintoCdcLinkManager.Visible changes:
control.sh --cdc resendfails while CDC is disabled; before, it wrote records to segments that were never linked.