PDGGK commented on PR #9180: URL: https://github.com/apache/paimon/pull/9180#issuecomment-5261424500
Correcting my own patch — the first version was wrong in two ways, and one of them was worse than the bug it fixed. Pushed 64fdd4e. Moving `bucketList.add()` out of the `computeIfAbsent` and into the `compute()` was not enough, because the `computeIfAbsent` is what creates the entry for a partition's **first** bucket. By the time `compute()` runs for it, `l` is not null, so that bucket was never registered either. | | | |---|---| | the first bucket froze at exactly its target while the others took all the overflow | still skew, just a different shape | | a partition starting **after** `maxBucketId` already sits at the cap never creates a second bucket, so its `bucketList` stays empty | `pickRandomly` throws `IllegalArgumentException: list is empty` — a crash where master merely skewed | A bucket is created by exactly one of the two calls, so both now register it and neither can double-register. **Two test changes, and the first one is the uncomfortable part.** My original spread assertion was "more than one bucket ends up past its target" — and that passes against the broken patch, because three of the four buckets did go past. Tightened to *every* bucket past its target. is new and is what catches the crash. Three-way run on the same test class: | | result | |---|---| | master | 1 failure | | my first patch | **2 failures** | | this | 12/12 pass | Apologies for the churn — the loose assertion is exactly the weakness I called out in `testAssignWithUpperBound` in the PR description, and I then shipped the same kind of assertion myself. -- 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]
