morningman commented on code in PR #69043:
URL: https://github.com/apache/doris/pull/69043#discussion_r4269728105
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/action/IcebergExpireSnapshotsAction.java:
##########
@@ -221,14 +229,14 @@ protected List<String> executeAction(Table icebergTable,
ConnectorSession sessio
// Set up delete callback to count files by type
expireSnapshots.deleteWith(path -> {
- FileContent deleteContent = deleteFileContentByPath.get(path);
- if (deleteContent == FileContent.POSITION_DELETES) {
+ FileKind fileKind = fileKindByPath.get(path);
+ if (fileKind == FileKind.POSITION_DELETES) {
deletedPositionDeleteFilesCount.incrementAndGet();
- } else if (deleteContent == FileContent.EQUALITY_DELETES) {
+ } else if (fileKind == FileKind.EQUALITY_DELETES) {
deletedEqualityDeleteFilesCount.incrementAndGet();
- } else if (path.contains("-m-") && path.endsWith(".avro")) {
+ } else if (fileKind == FileKind.MANIFEST) {
deletedManifestFilesCount.incrementAndGet();
- } else if (path.contains("snap-") && path.endsWith(".avro")) {
+ } else if (fileKind == FileKind.MANIFEST_LIST) {
Review Comment:
Fixed in 681c82d37c2. The map can miss snapshots, as you describe:
`buildFileKindMap` reads the snapshots of the table as it was loaded, and
`RemoveSnapshots.internalApply()` refreshes the metadata when it commits, so a
snapshot that another writer commits in between can expire with the others. Its
manifest list then fell through to `deleted_data_files_count`, where the old
`snap-` name check counted it correctly.
For a path the map does not know, the callback now falls back to the Iceberg
file name: `snap-*.avro` is a manifest list, and `<commit UUID>-m<N>.avro`
(`SnapshotProducer`) or `optimized-m-*.avro` (Spark's `rewrite_manifests`) is a
manifest. This also counts the manifests of such snapshots, which the old `-m-`
check missed. A delete file of such a snapshot still counts as a data file,
because its name does not say which kind it is.
Tests:
- `countsMetadataOfSnapshotsCommittedAfterTheTableWasLoaded` loads the
table. Another writer then appends, deletes and appends, and the action expires
snapshots through the loaded table. It uses an `older_than` in 2100 with
`retain_last=1`, so the result does not depend on the clock. It expects `[1, 0,
0, 2, 3, 0]`; without the fallback the result is `[5, 0, 0, 0, 1, 0]`.
- `classifiesUnmappedMetadataFilesByIcebergFileNames` covers the name rules,
including an Avro data file and a `snap-` directory, which must not match.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
