JingsongLi commented on PR #6426: URL: https://github.com/apache/paimon/pull/6426#issuecomment-4778563609
Hi @irodriguezclaveria, thanks for checking this carefully. You are right that, for the compaction-only case, if a bucket was loaded by `loadIndex` and later removed from `nonFullBucketInformation` only because it became full, then the bucket id is already in `totalBucketSet` / `totalBucketArray`, and its hashes have already been loaded into `hash2Bucket`. In that narrower model, we do not need to update the total bucket structures or reload hash mappings during refresh. The scenario I had in mind is the broader one where `refreshBucketsFromDisk` scans the latest index manifests and sees a bucket that was created after this `PartitionIndex` was loaded. That bucket may pass `bucketFilter`, but it is not in `totalBucketSet`, and its hash mappings were not read by the original `loadIndex`. If refresh adds such a bucket directly to `nonFullBucketInformation`, the in-memory state becomes inconsistent. Since this PR is about reusing buckets freed by compaction, not discovering newly-created buckets from another writer, I think the conservative fix is to make that invariant explicit: during refresh, only merge buckets that already exist in `totalBucketSet` (and still satisfy `bucketFilter`). Then the hash mapping argument holds as you described. If we later want refresh to import newly-created buckets too, then it should also update `totalBucketSet` / `totalBucketArray` and load the hash index contents for those buckets. For the concurrency/conservativeness question: off by default + 24h default interval + per-partition single-flight is enough for me. I do not think we need a separate executor or an additional global cap in this PR. I would just keep the refresh work bounded by the shared file-operation pool and avoid extra fan-out if possible. For tests, please make sure the regression really loads the partition before the later on-disk change. Then verify a known bucket that became full/was removed from `nonFullBucketInformation` can be re-added after compaction lowers its row count. A second test could add a new bucket after load and verify refresh does not import it, unless we explicitly decide to support that broader behavior. -- 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]
