Conversation
Intended to resolve #280
cb1kenobi
left a comment
cb1kenobi
left a comment
There was a problem hiding this comment.
Seems a little hacky, but I do love a good hack :)
Co-authored-by: Chris Barber <chris@harperdb.io>
Totally. :) Seemed correct enough for this specific test. |
| assert.equal(response.status, 200); | ||
| assert(!response.headers['server-timing'].includes('miss')); | ||
| assert.equal(response.data.name, 'name3'); | ||
| // loadAsInstance cache write commits in the background after the response |
There was a problem hiding this comment.
Yes, it does write commits in the background, but it retains a lock on the cache resolution until the write commits: https://github.com/HarperFast/harper/blob/main/resources/Table.ts#L4185
And the subsequent cache request should hit that lock here: https://github.com/HarperFast/harper/blob/main/resources/Table.ts#L3950
Which should force the second request to wait for the cached data to be written before checking the record again, at which point it should find a cached entry: https://github.com/HarperFast/harper/blob/main/resources/Table.ts#L3948 . So another miss immediately proceeding would indicate some problem (so polling would mask the problem). However...
I think maybe the problem is that it goes into the ensureLoadedFromSource/getFromSource code, which, even though it finds it in the cache, is still treated as a miss because it used
ensureLoadedFromSource to load it, and then marks it was loaded from source (a miss) here:
https://github.com/HarperFast/harper/blob/main/resources/Table.ts#L586
I think the right resolution is to pass the target through to the getFromSource so it can determine if we really ended up resolving from origin/source.
There was a problem hiding this comment.
@kriszyp not 100% sure on this one but it feels like it's in the right direction...
heskew
changed the title
3 participants
Intended to resolve #280