Clean up ordinal map in default SSDV reader state #12454
Conversation
Personally, I agree with this. Maybe we clean up the documentation and leave as-is? |
|
Thanks Greg! I’ve kept the hash map and did some clean-up. |
| long bytes = 0; | ||
| for (OrdinalMap map : cachedOrdMaps.values()) { | ||
| bytes += map.ramBytesUsed(); | ||
| synchronized (cachedOrdMap) { |
There was a problem hiding this comment.
We might not need this synchronization. Because the map has one entry at most and we're only writing it once, I don't think we can end up in an invalid state here, but it seemes wiser not to rely on that.
|
@gsmiller, I think this PR is ready. Is there anything else you'd like to see changed? |
Gah! I'm sorry I missed this. I'll have a look here shortly. Apologies again. |
|
Thanks Greg! I think the delay is partially my fault, I had mentioned a different G. Miller in my message 😄 |
Seems to be common mistake recently! See this recent hilarious example! |
DefaultSortedSetDocValuesReaderStatehas a cache of ordinal maps, but it only has one ordinal map in it at most.This PR preserves the functionality of the class, but removes
cachedOrdMaps. We can't synchronise on the ordinal map itself, since it could benull, so we introduce an ordinal map lock instead.Overall, I like the previous code a bit better, it's cleaner and more concise, although
cachedOrdMapslooks confusing at first. Maybe it just needs a comment explaining why using a map is convenient (we can synchronize on it, we can callAccountables.namedAccountableswith it)? I'd like to hear what others think.