oracledb_cdc: support controlling LogMiner session age - #4669
oracledb_cdc: support controlling LogMiner session age#4669josephwoodward wants to merge 7 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
96885e4 to
f9f6bf7
Compare
| if cfg.SessionMaxAge, err = lmConf.FieldDuration(ociFieldSessionMaxAge); err != nil { | ||
| return nil, err | ||
| } | ||
| if cfg.SessionMaxAge < 0 { |
There was a problem hiding this comment.
This new validation branch has no test coverage. internal/impl/oracledb/config_test.go already has the natural home for it — the table-driven errContains pattern used by TestBuildConnectionURL (config_test.go#L21-L32) and the oracleDBStreamConfigSpec.ParseYAML setup in TestParseSnapshotMode (config_test.go#L219-L227).
Adding a case that parses logminer: {session_max_age: -5s} and asserts require.ErrorContains(t, err, "must be 0 or greater") (plus a positive case asserting the parsed duration reaches Config.SessionMaxAge) would cover both the guard and the field wiring. The new session_test.go tests exercise the LogMiner side of the feature but not the config path.
| lm.log.Debugf("LogMiner session has been open for %s, exceeding session_max_age of %s — ending idle session to release accumulated session memory", | ||
| lm.sessionMgr.Age(), lm.cfg.SessionMaxAge) | ||
| if err := lm.sessionMgr.EndSession(ctx, conn); err != nil { | ||
| return fmt.Errorf("ending expired logminer session while idle: %w", err) |
There was a problem hiding this comment.
Idle END_LOGMNR failure now tears down the whole input, unlike the existing restart path.
endExpiredIdleSession propagates the EndSession error, and miningCycle returns it to ReadChanges, which aborts the streaming goroutine. In input_oracledb_cdc.go that error triggers o.stopSig.TriggerHasStopped(), so Read returns service.ErrNotConnected (L726-L730) and the connector performs a full reconnect/Connect() cycle.
The identical call in the age/log-switch restart path is deliberately non-fatal — it logs and continues:
connect/internal/impl/oracledb/logminer/logminer.go
Lines 1157 to 1162 in 042d405
Ending an idle session is purely a PGA-relief optimisation, so a transient END_LOGMNR failure shouldn't restart the input. Suggested fix: log at error/warn level and return nil here, matching the restart path. Retry is automatic — EndSession leaves active/sessionOpened untouched on failure, so IsExpired stays true and the next mining cycle tries again. This asymmetry between two invocations of the same operation is the kind of inconsistency CONTRIBUTING.md §3.1.5 (consistency with other connectors) and §3.2.2 (poor error handling) call out.
| return nil, err | ||
| } | ||
| if cfg.SessionMaxAge < 0 { | ||
| return nil, fmt.Errorf("logminer.%s must be 0 or greater, got %s", ociFieldSessionMaxAge, cfg.SessionMaxAge) |
There was a problem hiding this comment.
New config validation branch has no test.
This is the only new user-facing validation added by the PR, and nothing exercises it. internal/impl/oracledb/config_test.go already has the table-driven ParseYAML + errContains harness this fits into (see TestBuildConnectionURL and TestParseSnapshotMode), so adding a parseLogMinerConfig case with logminer.session_max_age: -1s asserting must be 0 or greater is cheap.
Per the project test patterns, changed code should not land without tests; a session_max_age: 0s / omitted case would also pin the documented default (§1.2.4, strong config validation).
1 participant
No description provided.