[Managed Iceberg] Fix partition value race condition #33549
[Managed Iceberg] Fix partition value race condition #33549ahmedabu98 merged 4 commits intoapache:masterfrom
Conversation
kennknowles
left a comment
kennknowles
left a comment
There was a problem hiding this comment.
It may be worth doing more to signpost what is mutable and what is not. I'm not sure what that would look like. But this change is really subtle even though it is small. It is easy to imagine we have other issues and/or will have issues in the future.
| routingPartitionKey.partition(getPartitionableRecord(record)); | ||
|
|
||
| if (!writers.asMap().containsKey(partitionKey) && openWriters >= maxNumWriters) { | ||
| if (!writers.asMap().containsKey(routingPartitionKey) && openWriters >= maxNumWriters) { |
There was a problem hiding this comment.
nit: You aren't guaranteed that asMap is cheap. It could be linear time to convert from some other underlying data structure. It is better to call get and check if the result was null. (since this is not a LoadingCache you don't have to worry about accidentally creating too many entries)
(maybe think about it for future changes)
There was a problem hiding this comment.
Thanks for the tip, will do
|
Assigning reviewers. If you would like to opt out of this review, comment R: @damondouglas for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
* fix and update tests * dont mention df yet * add PR link * whitespace
* fix and update tests * dont mention df yet * add PR link * whitespace
2 participants
Fixes #33497 (see comment and comment for details)
Also updates tests