fapifta commented on issue #705: HDDS-3249: renew ContainerCache.INSTANCE in order to test it in a fresh state URL: https://github.com/apache/hadoop-ozone/pull/705#issuecomment-602964815 Oh, that gives some better understanding the problem, which is the size of the cache, thank you for enlighten me on that, and for the follow up. Also thank you for adding the PR description. This approach is way more better, but I still have some problems but their roots are not in the fix you provided. Let's see what the TestContainerCache actually does... it tests whether: 1. the cache returns the same object with increased reference count with the same parameters 2. even if we got two references we have space for a third one with cache size 2 as the first two value we got are the same and it is not cached twice 3. after the second distinct element was get from the cache, the cache is full 4. eviction happens when the reference count is zero and we can have a new element cached instead of the one with zero reference 5. eviction does not happen early, so even if reference count is 0 for an element, if we get it again before evicted, we get the same object again with reference count 1 6. ensures that ReferenceCountedDb can not decrement the reference count below zero without an error. From here this is just my opinion, and I would like to understand what do you think about it. I would argue whether we need this test at all... My argument is that in these cases we test the fact that LRUMap is behaving as it is documented, and this is not in the scope of our test. Our TestContainerCache should ensure that we extended the functionality of LRUMap correctly, so we should test the removeLRU method, which is basically the test of ReferenceCountedDB.cleanup, which again is not in the scope for this class, however test that a reference we did not closed is not removable is good to have, to ensure this invariant is met even if functionality changes. We can test the first assumption of the original test as it is ensuring that we are having a cache and we are counting references properly when one reference is obtained to the ReferenceCountedDB instance. We can ensure that getDB and removeDB works as expected via tests, so it returns the cached instance, and removed the cached instance only when it is removable (~: referencecount=0). Also we can test the cache shutdown whether it clears the cache for example, but we certainly don't need to test isFull and things like that. SideNote: the current design for me is a bit unfortunate, but this should be a subject of a follow up JIRAs which if you create I happy to collaborate in. The problems I see: - by extending the LRUMap and not hiding its original functionality, getting the instance of ContainerCache allows a user to alter the cache causing weird behaviour if misused or misunderstood. - we have synchronization on a lock in all methods, why we do not have these methods as synchronized then? - we use this class only from BlockUtils, we should have the BlockUtils methods implemented in a way that it uses an internal LRUMap instead of the ContainerCache where we do not add much functionality? - ultimately, as we also doing instance creation in the getDB method of ContainerCache, why we are not using a LoadingCache instead with a simple size restriction, and forget reference counting on the db, and just re-create an instance if it was evicted from the cache?
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
