sodonnel commented on pull request #1549:
URL: https://github.com/apache/ozone/pull/1549#issuecomment-728098618
@elek thanks for taking a look.
Are you concerned about the exception handler:
```
} finally {
if (cachedDB != null) {
// If we get a cached instance, calling close simply decrements the
// reference count.
cachedDB.close();
} else if (store != null) {
// We only stop the store if cacheDB is null, as otherwise we would
// close the rocksDB handle in the cache and the next reader would
fail
try {
store.stop();
} catch (IOException e) {
throw e;
} catch (Exception e) {
throw new RuntimeException("Unexpected exception closing the " +
"RocksDB when loading containers", e);
}
}
}
```
Or that we may have an uncached instance DatanodeStore or a cached Store,
which comes out as a ReferenceCountedDB?
The code messiness is all driven from the static container cache.
> Can we use uncached db in both cases? In that case It would be possible to
always created uncached data.
Yes, the intention is to always use uncachedDB in both these cases, and
provided the Datanode is started from a cold JVM or the container has never
been loaded yet (import case) all will be fine. The original version did this
and it was much cleaner, but lots of tests failed due to the static
containerCache. If the RocksDB is already open in the cache, you cannot open
another handle to it or it will throw an exception. So in the test case with
MiniClusters, the code will try to get an uncachedDB, but it may fail due to
being open already. With the current logic, when it fails it will get the
instance from the cache. This makes the cleanup in the finally block messy, as
we have to handle both cases.
I did have another attempt at this change, where I try to always return a
ReferenceCountedDB from getUncached and handle the exception within that method
- however at the end you need to decrement the reference and close the DB, but
only if it did not come from the cache. If the handle came from the cache, you
should just decrement and not close. At that point, you don't know which is
which and cannot make a decision.
> If not, we can add DatanodeStore as a parameter of
KeyValueContainerUtil.parseKVContainerData and you can always decide which
store is used (cached / uncached)
That probably won't make it any cleaner. We always want to pass an
uncachedDB, but in the test case, we will need to sometimes fall back to the
cache. Then we need to increment and decrement the reference count and then
maybe call close. It just pushes the same messy logic elsewhere.
Another thing I thought of, was to check if the RocksDB is already cached.
However that involves taking a read lock on the ContainerCache. It also does
not have an existing method in the interface to make this check, and calling
get(...) returns or creates a new entry in the cache. All this logic would be
needed only for the tests and would introduce a small contention on the lock,
and avoiding that is the point of this change. In other words, we would be
checking for existance at runtime and never find anything, unless we are in a
mini-cluster.
----------------------------------------------------------------
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
