[cache] Make an aborted insert operation call the callback with false#23136
[cache] Make an aborted insert operation call the callback with false#23136mattklein123 merged 2 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
Code looks fine. But I'm curious what the callback would do with a 'true' value from insert. That doesn't guarantee a subsequent lookup would pass, right? An eviction might occur immediately after a successful insert.
| cache_.insert(key_, std::move(response_headers_), std::move(metadata_), body_.toString(), | ||
| std::move(trailers_)); | ||
| return cache_.insert(key_, std::move(response_headers_), std::move(metadata_), | ||
| body_.toString(), std::move(trailers_)); |
There was a problem hiding this comment.
this didn't really change this PR but should we be thinking about a zero-copy way to transfer the body?
There was a problem hiding this comment.
I think simple_http_cache is just supposed to be an example, not a serious implementation. But also I think zero-copy isn't really an option anyway because the original version is still being used for the response - you can't have one used for the response and a separate one for the cache with no copying. (And you can't use the same one because the response path is going to destroy it.)
|
also CI needs to be addressed. /wait |
I don't think there's a strong implication of "true" - it means the part of the insert operation was completed, so [nothing important]. The implication of false, however, is meaningful in that it means the entry wasn't inserted, or was rejected for some reason before even reaching the end of the insert operation, which could potentially allow the filter to stop trying to insert the rest of the same record. (But right now the filter doesn't do anything with the callback parameter's value, it's only used in tests.) So a "true" response means "don't do that", essentially. :) |
|
As far as I can tell the CI failures were all a failure to download something, not related to the change. Pushing a non-change to trigger testing again and clear the |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
Makes sense to me on all fronts. Thanks. |
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
3 participants
Commit Message: [cache] Make an aborted insert operation call the callback with false
Additional Description: The callback's parameter is currently only used in testing (though it could potentially be used to abandon an insert operation early); the previous implementation asserted that the insert callback was called with true, before checking that the insert did not in fact complete, which doesn't really make a lot of sense. The reasonable fixes were either to have the test ignore the callback's value and have it only check that the insert didn't actually happen, or validate that the return value correctly reflects an uncompleted insert operation, which involved also making simple_http_cache fulfill that expectation.
Risk Level: Very low (mostly test-only change to WIP extension)
Testing: Mostly a change to only test-facing behavior.
Docs Changes: n/a
Release Notes: May require a corresponding change to any existing external cache implementations in order for them to pass the generic cache test suite.
Platform Specific Features: n/a