Cache client partition results to save the repeated sort() calls #287
Conversation
|
|
||
| // If the number of partitions is large, we can get some churn calling cachedPartitions, | ||
| // so the result is cached. It is important to update this value whenever metadata is changed | ||
| cachedPartitionsResults map[string]map[int][]int32 |
There was a problem hiding this comment.
Given there are only two possible values I'm not sure using a nested map is really necessary
|
I pushed a new change that uses an array rather than a map. I think not returning a copy does not let people modify the result, but it also saves on memory allocations. I think this behavior should be ok(?) Not sure why someone would depend upon that. |
|
|
||
| // If the number of partitions is large, we can get some churn calling cachedPartitions, | ||
| // so the result is cached. It is important to update this value whenever metadata is changed | ||
| cachedPartitionsResults map[string][][]int32 |
There was a problem hiding this comment.
I was thinking two separate maps (or even a fixed array [2]int32) but I guess a slice is just as good
There was a problem hiding this comment.
I'd prefer to use a static array here, because it makes it more clear that this is not a list but a tuple.
Yes it does, which is a good point.
Neither am I (we don't, as far as I am aware) but I've learned to expect to be surprised :) I've mentioned a few style nits, but I don't think there's anything really wrong with this as it stands. @Shopify/kafka for an additional review please? |
| return client.cachedPartitionsResults[topic][partitionSet] | ||
| } | ||
|
|
||
| func (client *Client) cachedPartitionsNoLock(topic string, partitionSet partitionType) []int32 { |
There was a problem hiding this comment.
I find this function name slightly confusing; based on the name I expect cachedPartitions to simply call grab the lock and call this function.
Maybe setPartitionCache or updatePartitionCache?
|
This looks good to me; just some comments about naming to make the intent clearer. |
For more information, see #286
|
I've pushed one more version based upon the comments. If there are more thoughts on names, it's ok if you want to pull my branch and make them before pushing upstream. |
|
This looks great now. Thanks for your contribution! |
Cache client partition results to save the repeated sort() calls
For more information, see #286