Rewrite RR cache access time update to preserve table insert order #7578
Conversation
The cache ETS table is a bag table that in itself preserves insert order, and when access times in the table are update, it is possible to do the insertion of RRs with updated access time in the same order, which overall preserves insert order. To preserve the RR order; rewrite the deduplication code to not store the deduplicated result in a map, but instead use a map for deduplication lookup and store the result in a list. All this to not get in the way of DNS servers that e.g. implement load balancing by returning RRS in some specific order.
RaimoNiskanen
added
team:PS
CT Test Results 2 files 65 suites 1h 1m 15s ⏱️ For more details on these failures, see this check. Results for commit 49e7325. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
|
@RaimoNiskanen LGTM! I was quite convinced there was an edge case that would not afford simply switching to list for everything to just work ™️ Thanks for showing me otherwise ❤️ Edit: I fired off too quick bad instrumentation. Let me double check things again, but I'm seemingly still ending up with a sorted list of entries. |
|
Ok, double checked, triple checked even 😄, and it looks solid 👍 Here's a gist of the test. |
|
It would also be nice if @mattbaker could verify that it solves his #7577, and if @sneako could verify that his heavy load cache tests does not suffer regression... |
Wow, exciting! Thank you for working on this. I just got back from vacation so I have a bit of catch up to do, but I will make sure to test this out this week. |
|
First time compiling Erlang but I think I got it 😅 I can confirm that records no longer appear sorted with this patch, looks like a success to me 👍 |
|
I've been running this patch on a single node in a cluster for 2 hours so far and performance looks identical to the unpatched peers. Have not noticed any adverse side effects. LGTM, thanks everyone! |
RaimoNiskanen
deleted the
raimo/kernel/inet_dns-stable-cache/GH-7577/OTP-18731
branch
The cache ETS table is a bag table that in itself preserves insert order, and when access times in the table are update, it is possible to do the insertion of RRs with updated access time in the same order, which overall preserves insert order.
To preserve the RR order; rewrite the deduplication code to not store the deduplicated result in a map, but instead use a map for deduplication lookup and store the result in a list.
All this to not get in the way of DNS servers that e.g. implement load balancing by returning RRs in some specific order.
Closes #7577