fix: update map rather than create a new map#3302
Conversation
Signed-off-by: beanz <[email protected]>
There was a problem hiding this comment.
N.B. that adding a capacity hint to make(map[K]V) is merely a hint, and could be ignored by the compiler/runtime…
Also, it’s still better that we provide it when we have it.
PS: most of my comments are merely further improvements, rather than code defects
admin.go
Outdated
| for broker, partitions := range partitionPerBroker { | ||
| topics := make(map[string]*DeleteRecordsRequestTopic) | ||
| recordsToDelete := make(map[int32]int64) | ||
| topics := make(map[string]*DeleteRecordsRequestTopic, 1) |
There was a problem hiding this comment.
Move this down to line 623, and convert it and lines 623–625 to a complex literal.
admin.go
Outdated
| groups := make(map[string]string, len(response.Groups)) | ||
| maps.Copy(groups, response.Groups) |
There was a problem hiding this comment.
| groups := make(map[string]string, len(response.Groups)) | |
| maps.Copy(groups, response.Groups) | |
| groups := maps.Clone(response.Groups) |
There was a problem hiding this comment.
Rather than just do this one, I decided to make an additional commit with all of the uncontentious changes from your earlier comments on #3297.
One of the changes, which looks logically correct to me, causes a linter error so I may live to regret making the wider changes.
hindessm
force-pushed
the
known-map-sizes
branch
from
5ed580a to
d0a81d5
Compare
balance_strategy.go
Outdated
| currentAssignment = deepCopyAssignment(preBalanceAssignment) | ||
| currentPartitionConsumer = make(map[topicPartitionAssignment]string, len(preBalancePartitionConsumers)) | ||
| maps.Copy(currentPartitionConsumer, preBalancePartitionConsumers) | ||
| currentPartitionConsumer = maps.Clone(preBalancePartitionConsumers) |
There was a problem hiding this comment.
Huh. This was even called out when it was implemented: https://github.com/IBM/sarama/pull/1416/files#r314581692
Turns out, the code it’s copying used currentPartitionConsumer.clear() which deletes all the entries from the underlying map referenced, rather than assigning a whole brand new map into it. In this way, the previous references to the map would reflect these updates, even when they’re not directly used later in this function. However, this code just constructs a whole new map, rather than clearing out the existing map, so that other references to the same map do not reflect the changes made… different maps.
The whole StickyAssignor.java file is completely different now… but updating to copy that code is obviously out of scope.
I would instead recommend here:
clear(currentPartitionConsumer)
maps.Copy(currentPartitionConsumer, preBalancePartitionConsumers)
This should be the intended original code, rather than this subtly bugged(?) code.
Co-authored-by: Cassondra Foesch <[email protected]> Signed-off-by: Mark Hindess <[email protected]>
These are the uncontentious changes mentioned after the merge of the earlier modernize PR. See: IBM#3297 (review) Co-authored-by: Cassondra Foesch <[email protected]> Signed-off-by: beanz <[email protected]>
Thanks to Cassondra for figuring out how this was intended to work. Co-authored-by: Cassondra Foesch <[email protected]> Signed-off-by: beanz <[email protected]>
hindessm
force-pushed
the
known-map-sizes
branch
from
ea9f589 to
735af1d
Compare
hindessm
changed the title
hindessm
changed the title
|
I can find nothing further to comment about. |
|
FYI: Somewhat related to this, I added a comment about the use of maps in the API to https://github.com/IBM/sarama/wiki/Ideas-that-will-break-backwards-compatibility. |
I don’t seem to see any way to add any discussion of things, though? Like:
(Saving further comments for a better place to discuss.) |
3 participants
This PR was originally just minor refactoring to improve map allocations. But a bug was found while responding to the excellent PR feedback from Cassondra.
Cassondra rightly points out that the explicit capacities are hints so don't always reduce (re)allocations. In fact, particularly when the capacities are small, they make no difference, but it is still good practice to be explicit when we know the size.