OAK-10494 - Add cache to reduce number of remote blobstore calls. #1155
Conversation
|
@amit-jain Could you have a look when you have time? Thanks a lot! The test is kinda stupid but I couldn't think of a better one... |
|
@ahanikel the PR looks good but I am not sure if the invalidation also has to happen somewhere. Overall, I think the call need not go to the backend in case the the corresponding file is available in the cache. |
|
@amit-jain I think the least recently used entries in the cache automatically fall off the cliff when the maxSize is reached, but I've still added an expiration of 15 minutes, so we don't waste memory unnecessarily. I've added my test to the tests in CachingDataStoreTest. I've also tried to use Mockito but it does not seem to work in this case (modifying the class variable seems to be ignored somehow in the mocking process).
Yes, but my understanding is that we try to avoid loading the blob if we don't need it, and so if we only call |
| @@ -638,4 +642,165 @@ private void waitFinish() { | |||
| e.printStackTrace(); | |||
| } | |||
| } | |||
| private AbstractSharedCachingDataStore createDataStore() throws Exception { | |||
There was a problem hiding this comment.
Can we not use the existing dummy objects to construct the datastore. For the variables we can create instance variables instead of static variables to make it easier to inject.
There was a problem hiding this comment.
@amit-jain I don't know what I was thinking when using static variables for the recordCache configuration, sorry for that. I'm using the existing test datastore now, but I had to adapt it a bit for simulating a delay when doing (remote) backend requests. Could you have another look? Thank you very much!
| LOG.trace("" + dataStore.getRecordIfStored(di)); // LOG.trace to avoid the call being optimised away | ||
| } | ||
| long timeCached = System.nanoTime() - start; | ||
|
|
There was a problem hiding this comment.
Ah! Now I understood why you are adding a delay. The assertion with time can be quite fragile. Can't we just assert on the cache object not being empty and presence of the record with identifier?
There was a problem hiding this comment.
Yes, I wanted to "prove" the effectiveness of the cache but although I've only checked for a 5x improvement where the difference should be at least 100x for that test case, I agree it is still fragile and should be avoided.
I've changed the test to only check that the record is in the cache after the first access, and to ensure that it is loaded from the cache when accessed a second time. Is that what you meant?
There was a problem hiding this comment.
yes. If you need to add test performance numbers maybe you can also add to oak-benchmarks.
| assertNotNull("Record with id " + id + " should be in the recordCache now", | ||
| dataStore.recordCache.get().getIfPresent(id)); | ||
| // make sure the record is loaded from the cache | ||
| backend.deleteRecord(di); |
There was a problem hiding this comment.
I think it's better to go through the AbstractSharedCachingDataStore#deleteRecord. That way we can assert that the record is not available in the cache as well. AbstractBacked#deleteRecord wouldn't be called directly imo.
There was a problem hiding this comment.
@amit-jain The thing is that I'm deleting the record from the cache in AbstractSharedCachingDataStore#deleteRecord as well: https://github.com/ahanikel/jackrabbit-oak/blob/issues/OAK-10494/oak-blob-plugins/src/main/java/org/apache/jackrabbit/oak/plugins/blob/AbstractSharedCachingDataStore.java#L331 so the following assert would fail.
The reason I'm deleting from the backend here is just to prove that the record is actually read from the cache.
There was a problem hiding this comment.
Ah, you meant to make sure the record is not read from the existing cache either? I've added an invalidate for that in the commit below.
There was a problem hiding this comment.
What I meant was if you call AbstractSharedCachingDataStore#deleteRecord. then invalidate is called implicitly and the test then also covers that change.
| dataStore.cache.invalidate(id); | ||
| assertNull("Record with id " + id + " should not be in the backend anymore", | ||
| backend.getRecord(di)); | ||
| assertNotNull("The record could not be loaded from the cache", |
There was a problem hiding this comment.
this has to be changed to assertNull because would have been called.
There was a problem hiding this comment.
Ah, you want to make sure that the record is no longer in the cache after deletion, right? I'll add a check for that in the next commit. The check above is to ensure that the record is loaded from the cache without using the backend when accessed a second time. That's why I'm deleting it from the backend, that way I can be sure it comes from the cache.
The cache size should be made configurable and I should probably add some tests, too, but we can still start the discussion.