github-actions[bot] commented on code in PR #66913:
URL: https://github.com/apache/doris/pull/66913#discussion_r3871643612
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalMetaCache.java:
##########
@@ -282,7 +316,7 @@ public View getIcebergView(ExternalTable dorisTable) {
}
public IcebergSchemaCacheValue getIcebergSchemaCacheValue(NameMapping
nameMapping, long schemaId) {
- IcebergTableCacheValue tableValue =
tableEntry.get(nameMapping.getCtlId()).get(nameMapping);
+ IcebergTableCacheValue tableValue = statementValue(nameMapping);
Review Comment:
[P1] Keep schema conversion on the leased catalog context
This now keeps the statement's G1 table generation alive across reset, but
only the raw `Table` is passed into the schema loader.
`IcebergUtils.getSchema()` then re-reads the live catalog authenticator plus
`enableMappingVarbinary` and `enableMappingTimestampTz`, so a miss can parse a
retained G1 schema under G2 settings/credentials (or fail while G1 is still
valid). The UUID/schema-id key does not encode those mapping flags. Carry the
value's captured authenticator and immutable mapping settings through the
complete schema read/parse, with a reset-between-lease-and-miss regression.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hudi/source/HudiScanNode.java:
##########
@@ -521,23 +591,31 @@ private void getPartitionsSplits(List<HivePartition>
partitions, List<Split> spl
@Override
public List<Split> getSplits(int numBackends) throws UserException {
- if (incrementalRead && !incrementalRelation.fallbackFullTableScan()) {
- return getIncrementalSplits();
- }
- initPrunedPartitions();
- List<Split> splits = Collections.synchronizedList(new ArrayList<>());
+ ensureHmsRuntimeGeneration();
try {
+ if (incrementalRead &&
!incrementalRelation.fallbackFullTableScan()) {
+ List<Split> splits = getIncrementalSplits();
+ ensureHmsRuntimeGeneration();
+ return splits;
+ }
+ acquireFsView();
Review Comment:
[P1] Skip the fs-view sync when full-scan pruning is empty
At this point `isBatchMode()` has already initialized `prunedPartitions`;
with zero partitions it selects this eager path, but this call acquires the fs
view and `tryAcquire()` synchronously runs `fsView.sync()` before the empty
list submits zero tasks. A predicate-pruned query (or a table with no completed
instant) that is already known to return no splits can therefore still block or
fail on unrelated remote timeline/storage I/O. Check the initialized empty set
before acquiring the view, and cover both ordinary empty cases with a blocking
or throwing `sync()`.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -869,7 +871,7 @@ public void dropColumn(ExternalTable dorisTable, ColumnPath
columnPath, long upd
@Override
public void updateTableProperties(ExternalTable dorisTable, Map<String,
String> properties, long updateTime)
throws UserException {
- Table icebergTable = IcebergUtils.getIcebergTable(dorisTable);
+ Table icebergTable = IcebergUtils.getWritableIcebergTable(dorisTable,
this);
Review Comment:
[P1] Bound the writable table's FileIO lifetime
This changed route performs a fresh direct `ops.loadTable()` through
`getWritableIcebergTable()`, but that helper returns the raw table without a
load guard or the `tableCleanup()` used by cache-owned loads. For Glue/S3
Tables and REST catalogs with vended per-table IO, a successful or failed
property update therefore drops the table without ever closing its FileIO. Run
the mutation under an operation-scoped generation owner that closes table-owned
IO in `finally` after commit/failure, and add close-counting coverage for both
exits.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalMetaCache.java:
##########
@@ -490,24 +645,51 @@ private SchemaCacheValue
loadSchemaCacheValue(IcebergSchemaCacheKey key, Table r
private void retireTableGeneration(NameMapping nameMapping,
@Nullable IcebergTableCacheValue previousValue,
IcebergTableCacheValue currentValue) {
- if (previousValue != null &&
previousValue.isSameOperationalGeneration(currentValue)) {
+ if (previousValue == null) {
return;
}
- MetaCacheEntry<IcebergSnapshotEntryKey, IcebergSnapshotCacheValue>
snapshots =
- snapshotEntry.getIfInitialized(nameMapping.getCtlId());
- if (snapshots != null) {
- // Projections of another metadata generation are unreachable.
Projections of the same
- // generation frozen on a previous handle keep that handle's
FileIO (vended credentials)
- // and location provider; scans bind to them, so they must be
rebuilt from the new handle.
- snapshots.invalidateIf((key, value) ->
key.getNameMapping().equals(nameMapping)
- && (!key.belongsTo(currentValue) ||
!sharesOperationalResources(currentValue, value)));
+ try {
+ if (previousValue.isSameOperationalGeneration(currentValue)) {
+ return;
+ }
+ MetaCacheEntry<IcebergSnapshotEntryKey, IcebergSnapshotCacheValue>
snapshots =
+ snapshotEntry.getIfInitialized(nameMapping.getCtlId());
+ if (snapshots != null) {
+ // Projections of another metadata generation are unreachable.
Projections of the same
+ // generation frozen on a previous handle keep that handle's
FileIO (vended credentials)
+ // and location provider; scans bind to them, so they must be
rebuilt from the new handle.
+ snapshots.invalidateIf((key, value) ->
key.getNameMapping().equals(nameMapping)
+ && (!key.belongsTo(currentValue) ||
!sharesOperationalResources(currentValue, value)));
+ }
+ Optional<String> currentUuid = currentValue.getTableUuid();
+ MetaCacheEntry<IcebergSchemaCacheKey, SchemaCacheValue> schemas =
+ schemaEntry.getIfInitialized(nameMapping.getCtlId());
+ if (schemas != null) {
+ schemas.invalidateIf(key ->
key.getNameMapping().equals(nameMapping)
+ && !key.getTableUuid().equals(currentUuid));
+ }
+ } finally {
+ // Caffeine REPLACED notifications intentionally do not run the
removal listener because
+ // the cache reservation transfers to the new generation. Resource
ownership does not:
+ // retire the old value here and let active statement/async leases
delay physical close.
+ previousValue.retire();
Review Comment:
[P1] Do not retain a projection after closing its exact FileIO
When the replacement has a different FileIO instance with equal
properties/credentials, `isSameOperationalGeneration()` returns true, so this
method keeps the existing snapshot projection; this new `finally` still retires
the previous value and closes its table-owned IO. `FrozenTableOperations`
captured that exact old IO object, so a later statement can retrieve a
still-cached projection backed by a closed client. Invalidate on FileIO
identity change or give the projection a counted owner for the old value, and
make the equivalence test use close-tracking IO.
--
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]
