Ensure cache gets properly updated with concurrent access for action with attachments #4183
Conversation
This file contains 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
markusthoemmes
approved these changes
Dec 17, 2018
| @@ -198,23 +198,40 @@ trait DocumentFactory[W <: DocumentRevisionProvider] extends MultipleReadersSing | |||
| doc: DocId, | |||
| rev: DocRevision = DocRevision.empty, | |||
| fromCache: Boolean = cacheEnabled)(implicit transid: TransactionId, mw: Manifest[W]): Future[W] = { | |||
| getWithAttachment(db, doc, rev, fromCache, None) | |||
| Try { | |||
| require(db != null, "db undefined") | |||
There was a problem hiding this comment.
I think we should get rid of all of these. Do we ever use null anywhere?
| Try { | ||
| require(db != null, "db undefined") | ||
| } map { | ||
| implicit val logger = db.logging | ||
| implicit val ec = db.executionContext | ||
| val key = doc.asDocInfo(rev) | ||
| _ => | ||
| cacheLookup(CacheKey(key), db.get[W](key, attachmentHandler), fromCache) | ||
| cacheLookup(CacheKey(key), db.get[W](key, Some(attachmentHandler)) flatMap { postProcess }, fromCache) |
There was a problem hiding this comment.
Nit: I find the combinators a bit hard to read
db.get[W](key, Some(attachmentHandler)).flatMap(postProcess)Is imo a bit clearer.
Codecov Report
@@ Coverage Diff @@
## master #4183 +/- ##
==========================================
- Coverage 86.03% 81.19% -4.85%
==========================================
Files 152 152
Lines 7292 7275 -17
Branches 472 477 +5
==========================================
- Hits 6274 5907 -367
- Misses 1018 1368 +350
Continue to review full report at Codecov.
|
BillZong
pushed a commit
to BillZong/openwhisk
that referenced
this pull request
Nov 18, 2019
…with attachments (apache#4183)
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.
@tysonnorris notices with concurrent action invocation scenario that same attachment is being fetched multiple times in case of cache miss.
Description
If multiple request try to load an action with attachment which is not present in cache then currently it leads to multiple request to load the same attachment from backing store. This PR adds a new test which demonstrate this behavior. This test would try to concurrently read action (5 threads). Without fix you would see logs like below which show that attachment is being read multiple times
As a fix now the cache update operation also includes the inline logic. So a cache update would be considered complete only when the action code is also inlined. This ensures even with concurrent reader threads only one thread performs the db access and others make use of the cache
Related issue and scope
My changes affect the following components
Types of changes
Checklist: