Refactor ResponseCache usage #3093
Conversation
Adds a `.wrap` method to ResponseCache which wraps up the boilerplate of a (get, set) pair, and then use it throughout the codebase. This will be largely non-functional, but does include the following functional changes: * federation_server.on_context_state_request: drops use of _server_linearizer which looked redundant and could cause incorrect cache misses by yielding between the get and the set. * RoomListHandler.get_remote_public_room_list(): fixes logcontext leaks * the wrap function includes some logging. I'm hoping this won't be too noisy on production.
richvdh
force-pushed
the
rav/response_cache_wrap
branch
from
41bf31b to
b78395b
Compare
|
I am not sure I would have all cache uses on log level INFO. |
Turns out that ObservableDeferred.observe doesn't return a deferred if the result is already completed. Fix handling and improve documentation.
Can we either keep it or remove it entirely please? It seems odd to only remove it from one function. |
|
@krombel: sorry, I meant to reply before merging. Thanks very much for the review here - your feedback is much appreciated. I agree that in most cases DEBUG would be enough, but certainly we've found the existing logging to be quite useful in understanding what's going on. The problem this PR introduces is that it's difficult to have INFO logging for some caches and DEBUG for others. Obviously we could solve that by making each cache use its own logger whose level can be configured, but having discussed this with erik, we feel like the extra logging is probably going to be less spammy than a bunch of the existing logging. So for now we're going to land it - if it starts to feel like it's excessive, we can revisit it to make the logging more flexible. |
|
Most people are running on INFO AFAIK. So setting it to DEBUG would cut off the information about cache usage which on the other hand might be useful.
Looks good to me |
Changes in synapse v0.28.0-rc1 (2018-04-26) =========================================== Bug Fixes: * Fix quarantine media admin API and search reindex (PR #3130) * Fix media admin APIs (PR #3134) Changes in synapse v0.28.0-rc1 (2018-04-24) =========================================== Minor performance improvement to federation sending and bug fixes. (Note: This release does not include state resolutions discussed in matrix live) Features: * Add metrics for event processing lag (PR #3090) * Add metrics for ResponseCache (PR #3092) Changes: * Synapse on PyPy (PR #2760) Thanks to @Valodim! * move handling of auto_join_rooms to RegisterHandler (PR #2996) Thanks to @krombel! * Improve handling of SRV records for federation connections (PR #3016) Thanks to @silkeh! * Document the behaviour of ResponseCache (PR #3059) * Preparation for py3 (PR #3061, #3073, #3074, #3075, #3103, #3104, #3106, #3107, #3109, #3110) Thanks to @NotAFile! * update prometheus dashboard to use new metric names (PR #3069) Thanks to @krombel! * use python3-compatible prints (PR #3074) Thanks to @NotAFile! * Send federation events concurrently (PR #3078) * Limit concurrent event sends for a room (PR #3079) * Improve R30 stat definition (PR #3086) * Send events to ASes concurrently (PR #3088) * Refactor ResponseCache usage (PR #3093) * Clarify that SRV may not point to a CNAME (PR #3100) Thanks to @silkeh! * Use str(e) instead of e.message (PR #3103) Thanks to @NotAFile! * Use six.itervalues in some places (PR #3106) Thanks to @NotAFile! * Refactor store.have_events (PR #3117) Bug Fixes: * Return 401 for invalid access_token on logout (PR #2938) Thanks to @dklug! * Return a 404 rather than a 500 on rejoining empty rooms (PR #3080) * fix federation_domain_whitelist (PR #3099) * Avoid creating events with huge numbers of prev_events (PR #3113) * Reject events which have lots of prev_events (PR #3118)
Adds a
.wrapmethod to ResponseCache which wraps up the boilerplate of a(get, set) pair, and then use it throughout the codebase.
This will be largely non-functional, but does include the following functional
changes:
drops use of _server_linearizer which looked redundant and could cause incorrect cache misses by yielding between the get and the setwraps the cache fetch/set completely in the linearizer to avoid the race conditionon production.