kerneltime commented on pull request #3091:
URL: https://github.com/apache/ozone/pull/3091#issuecomment-1086335457


   @szetszwo I am not sure how this code is expected to work overall.
   1. `ReferenceCountedDB` lives in the `ContainerCache` and currently the 
logic to open and close a DB is tied to the lifecycle of the cache entry. 
   2. Can you point out the documentation recommending that the DB should be 
closed on any exception.
   3. The main problem we have in our code is that RocksDB expects that any 
Java class that inherits `RocksObject` should be manually closed when done 
instead of depending on the JVM calling the finalizer which then frees up the 
memory allocated by the underlying JNI code. This code addresses some of the 
code paths but there are other places where we need to reason about our memory 
management. We currently do not have a leak but are depending on the JVM to 
eventually clean up the memory. 
   4. The segfaults witnessed by the code were related to the open and close 
path for RocksDB and this code change would not solve the problems we 
encountered.
   I think it is a worthwhile effort to fix our usage of RocksDB but we need to 
complete the model including cache lifecycle management. If the underlying DB 
is closed it should be evicted from the cache as well after the reference count 
has gone down to 0. Also, if not all Exceptions need the DB to be closed, we 
should then only close the DB for the Exceptions that cannot be recovered.
   ```
     public enum Code {
       Ok(                 (byte)0x0),
       NotFound(           (byte)0x1),
       Corruption(         (byte)0x2),
       NotSupported(       (byte)0x3),
       InvalidArgument(    (byte)0x4),
       IOError(            (byte)0x5),
       MergeInProgress(    (byte)0x6),
       Incomplete(         (byte)0x7),
       ShutdownInProgress( (byte)0x8),
       TimedOut(           (byte)0x9),
       Aborted(            (byte)0xA),
       Busy(               (byte)0xB),
       Expired(            (byte)0xC),
       TryAgain(           (byte)0xD),
       Undefined(          (byte)0x7F);
   ```
   NotFound shows up as an error code that is can be sent up as part of the 
RocksDBException, is the normal key get impacted with closing the DB on 
Exception? 
   ```
     public byte[] get(ColumnFamily family, byte[] key) throws IOException {
       try {
         return db.get(family.getHandle(), key);
       } catch (RocksDBException e) {
         close();
         throw toIOException("get " + bytes2String(key) + " from " + family, e);
       }
     }
   ```


-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to