ForwardMsgCache expiration#82
Merged
tconkling merged 20 commits intostreamlit:feature/hashingfrom Sep 17, 2019
Merged
Conversation
This value needs to be updated after a report's messages have been sent
…port_finished message
tvst
reviewed
Sep 13, 2019
Contributor
tvst
left a comment
There was a problem hiding this comment.
Still reviewing this, but wanted to send out some comments.
This is no longer accessed from anywhere but the server thread
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implements
ForwardMsgCacheexpiration on both the client and the server.global.maxCachedMessageAgeconfig value specifies the maximum age of a message in the cache. The age of a message is defined by the number of times a referencing report has finished running since the message was accessed by that report. So when a report finishes running for the first time, the age of all its cached messages is 1.report_run_countalongside eachReportSessionin its websocket->reportsession dictionary. The server increments this value each time it processes areport_finishedForwardMsg message for a report.report_finished, it also asks the cache to remove any expired messages. I haven't done anything fancy here - the cache just iterates through all its entries, does an age check, and deletes them if they're expired. @tvst, I know you were potentially concerned about performance issues with the "iterate the whole cache" strategy. We could add a simple timer to this function to collect local metrics to see if it's actually an issue?report_finishedForwardMsg, it performs the same cache expiration step. This means that the server and client's caches should stay in sync. (I haven't thought hard enough about whether it's possible for them to get briefly out of sync, but if they do, we have the new/messageendpoint as a fallback.)report_run_countfor that session. (The run-count incrementing is conditional on the status value of thereport_finishedmessage, which is set toFINISHED_WITH_COMPILE_ERRORwhen that happens.)There are client and server tests for all this new logic. But the easiest way to see the expiration in action is to run something like
examples/core/message_deduping.pyand watch the debug output on the server and the client. (You'll see logs for cache hits and misses - you should never see a miss! - on the client; and logs for sending cached message refs on the server.)st.dataframe()calls and rerun the report, the dataframe will not be removed from the cache because it won't be old enough. Re-adding those calls and rerunning the report should confirm this.