SEZ9 commented on PR #12789: URL: https://github.com/apache/seatunnel/pull/12789#issuecomment-6293458343
Both remaining items addressed, thanks @DanielLeens. **Issue 1 (WARN for legal-but-costly values).** Added at master start, above 256, naming both the configured value and the recommendation: ``` seatunnel.engine.checkpoint.monitor-history-size is set to 1024, above the recommended 256. The checkpoint overview is rewritten in full on every checkpoint event, so a window this large costs master CPU, network and backup traffic proportional to history size x running pipelines. Lower it unless you need that much history. ``` A warning rather than a tighter cap, because whether that cost is acceptable depends on the cluster's pipeline count and checkpoint interval, not on the number alone — a 512-entry window on a single-pipeline job with a 5-minute interval is harmless, and on twenty pipelines at 10s it is not. The threshold is documented next to the bound in both language versions. **Issue 2 (the untested `startMaster` wiring).** Taking your second option and accepting the gap. The factory-method extraction would work, but it changes production structure to make one line of wiring unit-testable, and I would rather the code read the way it does than be shaped by the test. The config path and the trimming behaviour are both covered; the uncovered surface is `getMonitorHistorySize()` appearing in that constructor call, which is visible in review. **On CI.** I re-ran only the failed job as you asked — `unit-test (11, ubuntu-latest)` — and attempt 2 is running now. For the record on why I agree it is unrelated: `SeaTunnelClientTest.testSetJobIdDuplicate` is in `seatunnel-engine-client`, a module this PR does not touch, and the other three `unit-test` combinations (11/windows, 17/ubuntu, 17/windows) passed on the same commit. Both of this PR's own tests were green in the failing job too — `YamlSeaTunnelConfigParserTest` 6/6 and `PipelineCheckpointOverviewTest` 3/3, which matches what I get locally. This push adds a commit, so a fresh run will start on the new head regardless. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
