hudi-agent commented on code in PR #19998:
URL: https://github.com/apache/hudi/pull/19998#discussion_r4050750220
##########
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:
🤖 Since the goal here is to release everything the DAO opens: the
`ColumnFamilyOptions` created in `getColumnFamilyDescriptor` (one per CF on
open, plus one per `addColumnFamily`) is never closed — both this `clear()` and
the one in `close()` just drop the descriptors. RocksJava 7.5.3 has no
finalizer on `AbstractNativeReference`, so that native memory is leaked per DAO
lifecycle. Pre-existing and small, but would it make sense to close
`descriptor.getOptions()` here and in `close()` as a follow-up?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
