Repository navigation
Conversation
dwangatt
changed the title
16b6bb1 to
86047bb
Compare
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Long-running maintenance needs progress visibility. The local JDK 8 run passed 89 Core tests and 19 Flink 1 integration tests. The upstream Flink 2 checks passed. The Core CI failures are the CSV regression tracked in #10999. The Spark 4 failures came from Hive metastore tests.
Please also cover the local orphan-cleanup path used by Spark and Flink with mode='local'. Its candidate listing, manifest reads, and file deletion continue after the reporters in the shared helpers close. A single reporter spanning the local operation would cover those stalls.
| } | ||
| } | ||
|
|
||
| private String progressContext() { |
There was a problem hiding this comment.
[P1] Redact storage credentials before writing progress logs
Path.toString() includes URI user-info. Java's Jindo adapter explicitly handles legacy OSS paths with user-info before making filesystem calls, so those paths can reach maintenance with the original authority intact. I verified with a synthetic oss://user:password@... path that a no-op ExpireSnapshotsImpl.expire() emits both values in all four INFO records. Please sanitize table, directory and file URIs at every new logging call site using SensitiveConfigUtils.sanitizeUri(path.toUri().toString()), and assert redaction in a log-capture test.
| @Override | ||
| public int expire() { | ||
| try (MaintenanceProgress progress = | ||
| new MaintenanceProgress(progressContext(), "evaluate-retention")) { |
There was a problem hiding this comment.
[P2] Keep short automatic expiration checks off the INFO and thread-creation path
Regular writer commits run snapshot expiration by default, synchronously unless configured otherwise. This starts a reporter even when retention selects an empty range. A normal no-op check emits four INFO records and the last user's close shuts the shared scheduler down. A smoke test of 100 sequential no-op expire() calls started 100 platform threads even with logging disabled. Please use a lifecycle-scoped or genuinely shared timer and keep fast/no-op checks quiet while preserving the heartbeat for slow retention evaluation.
| } | ||
| if (files.isEmpty()) { | ||
| try (MaintenanceProgress progress = | ||
| new MaintenanceProgress( |
There was a problem hiding this comment.
[P2] Aggregate orphan-cleanup diagnostics per Flink subtask
Each candidate directory gets its own START/END pair and scheduled task here. The manifest-list reader at line 205 does the same for each snapshot. On a subtask listing 100,000 directories, this adds 200,000 INFO records, even if every call finishes before the heartbeat interval. Serial calls can also create a fresh thread when they are the last reporter user. Please use a reporter scoped to the subtask's open/close, record active paths with startItem/finishItem, and aggregate counters across elements.
| ? "dry_run_files" | ||
| : "delete_calls_completed", | ||
| 1); | ||
| LOG.debug( |
There was a problem hiding this comment.
[P2] Preserve the file list for explicit distributed dry runs
The old INFO line was the per-file output of a distributed dry run. The Flink procedure returns only count and bytes, so moving this to DEBUG leaves users at normal log levels unable to see which paths would be removed. Please keep a redacted path list for explicit dry runs, either in their logs or through an equivalent audit output. Flink cleanup procedure
Purpose
Closes #10993.
Snapshot expiration and orphan-file cleanup can run for a long time without enough visibility to distinguish slow progress from a stalled job.
This change:
Tests
MaintenanceProgressTest(3 tests)