linliu-code commented on code in PR #19998:
URL: https://github.com/apache/hudi/pull/19998#discussion_r4051101710
##########
hudi-common/src/main/java/org/apache/hudi/common/util/collection/RocksDBDAO.java:
##########
@@ -115,73 +119,95 @@ private void init() {
managedDescriptorMap = new ConcurrentHashMap<>();
// If already present, loads the existing column-family handles
- final DBOptions dbOptions = new
DBOptions().setCreateIfMissing(true).setCreateMissingColumnFamilies(true)
+ this.dbOptions = new
DBOptions().setCreateIfMissing(true).setCreateMissingColumnFamilies(true)
.setWalDir(rocksDBBasePath).setStatsDumpPeriodSec(300);
this.statistics = new Statistics();
dbOptions.setStatistics(statistics);
- dbOptions.setLogger(new org.rocksdb.Logger(dbOptions) {
- @Override
- protected void log(InfoLogLevel infoLogLevel, String logMsg) {
- switch (infoLogLevel) {
- case DEBUG_LEVEL:
- log.debug("From Rocks DB : {}", logMsg);
- break;
- case WARN_LEVEL:
- log.warn("From Rocks DB : {}", logMsg);
- break;
- case ERROR_LEVEL:
- case FATAL_LEVEL:
- log.error("From Rocks DB : {}", logMsg);
- break;
- case HEADER_LEVEL:
- case NUM_INFO_LOG_LEVELS:
- case INFO_LEVEL:
- default:
- log.info("From Rocks DB : {}", logMsg);
- break;
- }
- }
- });
+ this.logger = new RocksDBLogger(dbOptions);
+ dbOptions.setLogger(logger);
final List<ColumnFamilyDescriptor> managedColumnFamilies =
loadManagedColumnFamilies(dbOptions);
final List<ColumnFamilyHandle> managedHandles = new
ArrayList<>(managedColumnFamilies.size());
FileIOUtils.mkdir(new File(rocksDBBasePath));
rocksDB = RocksDB.open(dbOptions, rocksDBBasePath,
managedColumnFamilies, managedHandles);
defaultWriteOptions = new
WriteOptions().setDisableWAL(disableWALForWrites);
- ValidationUtils.checkArgument(managedHandles.size() ==
managedColumnFamilies.size(),
- "Unexpected number of handles are returned");
- for (int index = 0; index < managedHandles.size(); index++) {
- ColumnFamilyHandle handle = managedHandles.get(index);
- ColumnFamilyDescriptor descriptor = managedColumnFamilies.get(index);
- String familyNameFromHandle = fromUTF8Bytes(handle.getName());
- String familyNameFromDescriptor = fromUTF8Bytes(descriptor.getName());
-
-
ValidationUtils.checkArgument(familyNameFromDescriptor.equals(familyNameFromHandle),
- "Family Handles not in order with descriptors");
- managedHandlesMap.put(familyNameFromHandle, handle);
- managedDescriptorMap.put(familyNameFromDescriptor, descriptor);
- }
+ registerColumnFamilies(managedColumnFamilies, managedHandles);
} catch (RocksDBException | IOException re) {
log.error("Got exception opening Rocks DB instance ", re);
+ closeOnInitFailure();
throw new HoodieException(re);
+ } catch (RuntimeException re) {
+ // The validation in registerColumnFamilies runs after RocksDB.open(),
so this path can have
+ // an open DB to release as well.
+ closeOnInitFailure();
+ throw re;
+ }
+ }
+
+ /**
+ * Validates the handles RocksDB returned against the descriptors asked for,
and registers them.
+ */
+ void registerColumnFamilies(List<ColumnFamilyDescriptor>
managedColumnFamilies,
+ List<ColumnFamilyHandle> managedHandles) throws
RocksDBException {
+ ValidationUtils.checkArgument(managedHandles.size() ==
managedColumnFamilies.size(),
+ "Unexpected number of handles are returned");
+ for (int index = 0; index < managedHandles.size(); index++) {
+ ColumnFamilyHandle handle = managedHandles.get(index);
+ ColumnFamilyDescriptor descriptor = managedColumnFamilies.get(index);
+ String familyNameFromHandle = fromUTF8Bytes(handle.getName());
+ String familyNameFromDescriptor = fromUTF8Bytes(descriptor.getName());
+
+
ValidationUtils.checkArgument(familyNameFromDescriptor.equals(familyNameFromHandle),
+ "Family Handles not in order with descriptors");
+ managedHandlesMap.put(familyNameFromHandle, handle);
+ managedDescriptorMap.put(familyNameFromDescriptor, descriptor);
}
}
+ /**
+ * init() runs from the constructor, so a throw leaves no reference for any
caller to close():
+ * everything opened so far has to be released here or it outlives the
failed DAO.
+ */
+ private void closeOnInitFailure() {
+ if (managedHandlesMap != null) {
+
managedHandlesMap.values().forEach(AbstractImmutableNativeReference::close);
+ managedHandlesMap.clear();
+ }
+ if (managedDescriptorMap != null) {
+ managedDescriptorMap.clear();
Review Comment:
Good catch — and already addressed in the current head (`86922bc3`); your
comment is against `e2d1cf1f`, the revision before it.
`descriptor.getOptions().close()` now runs on all three paths — `close()`,
the init-failure cleanup, and `dropColumnFamily`, which discarded the
descriptor while closing only the handle. Measured before the fix: `2 of 2`
descriptor `ColumnFamilyOptions` still open after `close()`.
Covered by `testCloseReleasesColumnFamilyDescriptorOptions`,
`testDropColumnFamilyReleasesDescriptorOptions`, and a general
`testCloseReleasesEveryReachableNativeHandle` that walks the DAO object graph
before `close()` and requires every native handle to be released after — it
reports the path, so this class of miss surfaces without anyone having to think
of it:
```
native handles still open after close():
[RocksDBDAO.managedDescriptorMap[default].columnFamilyOptions_
(ColumnFamilyOptions)]
```
All three fail without the change.
--
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]
