JingsongLi commented on PR #6426:
URL: https://github.com/apache/paimon/pull/6426#issuecomment-4644908131
I took another pass specifically from the perspective of whether this change
is intrusive to existing paths.
The default path looks mostly non-intrusive:
`dynamic-bucket.empty-bucket-threshold` defaults to `-1`, so existing jobs that
do not set the new option should not enter the async refresh branch. However,
once the option is enabled, the new code becomes part of the bucket assignment
state machine rather than a purely side-channel optimization. I think there are
still correctness risks before this is safe to merge:
1. Refreshed buckets are added only to `nonFullBucketInformation`, but not
to `totalBucketSet` / `totalBucketArray`.
In `PartitionIndex.refreshBucketsFromDisk`, newly discovered buckets are
merged with `nonFullBucketInformation.putIfAbsent(...)`, but the same buckets
are not added to the structures used by the new-bucket creation and fallback
paths. After such a refreshed bucket is consumed and removed from
`nonFullBucketInformation`, the later creation path can still see it as absent
from `totalBucketSet` and may try to create/reuse the same bucket id as if it
were new. That makes the in-memory bucket state inconsistent.
2. Refresh only imports bucket row counts, not the hash-to-bucket mapping.
The initial `loadIndex` path reads each hash index file and fills
`hash2Bucket`, which preserves the invariant that an existing key hash is
always assigned back to its original bucket. The async refresh path only uses
`IndexFileMeta.rowCount()` and does not read the hash index contents for newly
discovered buckets. If a refreshed bucket already contains keys written by
another writer or visible in a newer snapshot, this assigner can later treat
those hashes as unseen and assign them elsewhere.
3. The enabled path also shares the global `FileOperationThreadPool` for
manifest/index scans. That is probably acceptable if intentionally throttled,
but it means the feature can affect other file operations once enabled, so the
option and interval need to be conservative.
Given the above, I would suggest fixing the refresh merge so it updates all
bucket state consistently, and either loads hash mappings for refreshed buckets
or clearly proves that refreshed buckets can only be empty for this assigner.
It would also be good to add tests for: (a) a refreshed bucket becoming full
and not being re-created as a new bucket, and (b) an existing hash in a
refreshed bucket still being assigned back to that bucket.
--
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]
