ptlrs commented on code in PR #9813:
URL: https://github.com/apache/ozone/pull/9813#discussion_r3075970622
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedDBOptions.java:
##########
@@ -34,7 +34,6 @@ public class ManagedDBOptions extends DBOptions {
private final UncheckedAutoCloseable leakTracker = track(this);
private final AtomicReference<Logger> loggerRef = new AtomicReference<>();
- @Override
Review Comment:
Do we need to remove this? The RocksDB JNI still has a `setLogger` method
available.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBTable.java:
##########
@@ -99,16 +99,16 @@ public boolean isEmpty() throws RocksDatabaseException {
@Override
public boolean isExist(byte[] key) throws RocksDatabaseException {
rdbMetrics.incNumDBKeyMayExistChecks();
- final Supplier<byte[]> holder = db.keyMayExist(family, key);
- if (holder == null) {
- return false; // definitely not exists
- }
- final byte[] value = holder.get();
- if (value != null) {
- return true; // definitely exists
+ final Supplier<byte[]> valueSupplier = db.keyMayExist(family, key);
+ if (valueSupplier != null) {
+ final byte[] value = valueSupplier.get();
+ if (value != null) {
+ return true; // definitely exists
+ }
}
- // inconclusive: the key may or may not exist
+ // keyMayExist can be a false-negative in some RocksDB setups (for example,
+ // snapshot/checkpoint DBs). Verify via point-get to preserve correctness.
final boolean exists = get(key) != null;
Review Comment:
I am trying to understand more about this comment or how we can reach this
stage.
The `keyMayExist` documentation says that it will never return a
false-negative.
Are we trying to say here that a key may not exist in the
snapshot/checkpoint DB but may exist in the active DB? In that case the
`get(key)` is still running on the same db which would be the snapshot DB
instead of the active DB and again not find the key.
> If the key definitely does not exist in the database, then this method
> returns false, otherwise it returns true if the key might exist.
> That is to say that this method is probabilistic and may return false
> positives, but **never a false negative**.
##########
pom.xml:
##########
@@ -203,7 +204,9 @@
<reflections.version>0.10.2</reflections.version>
<reload4j.version>1.2.26</reload4j.version>
<restrict-imports.enforcer.version>3.0.0</restrict-imports.enforcer.version>
- <rocksdb.version>7.7.3</rocksdb.version>
+ <rocksdb.source.version>10.10.1</rocksdb.source.version>
+ <rocksdb.version>10.10.1</rocksdb.version>
+ <rocksdbjni.groupId>org.rocksdb</rocksdbjni.groupId>
Review Comment:
I would like to understand this as well. Do we need to parameterize this?
##########
hadoop-hdds/rocks-native/src/main/patches/rocks-native.patch:
##########
@@ -380,9 +381,9 @@ index 000000000..5ba8a82ee
+
+ rep_->file_.reset(new RandomAccessFileReader(std::move(file), file_path));
+
-+ FilePrefetchBuffer prefetch_buffer(
-+ 0 /* readahead_size */, 0 /* max_readahead_size */, true /* enable */,
-+ false /* track_min_offset */);
++ FilePrefetchBuffer prefetch_buffer(ReadaheadParams(),
++ !fopts.use_mmap_reads /* enable */,
++ false /* track_min_offset */);
+ if (s.ok()) {
Review Comment:
Do we know where the `raw_sst_file_reader` header and implementation files
are located?
This
[comment](https://github.com/apache/ozone/pull/6182#discussion_r1498510016)
mentions that the implementation was originally copied from `sst_file_dumper`
but that file doesn't seem to show any signature changes in `10.10.1`.
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -112,18 +113,23 @@ public static ManagedRocksDB openWithLatestOptions(
}
/**
- * Delete liveMetaDataFile from rocks db using RocksDB#deleteFile Api.
- * This function makes the RocksDB#deleteFile Api synchronized by waiting
- * for the deletes to happen.
- * @param fileToBeDeleted File to be deleted.
+ * Delete the SST file range from rocks db and wait for file deletion.
+ * @param columnFamilyHandle column family of the target sst file.
+ * @param fileToBeDeleted file metadata to be deleted.
* @throws RocksDatabaseException if the underlying db throws an exception
* or the file is not deleted within a time
limit.
*/
- public void deleteFile(LiveFileMetaData fileToBeDeleted) throws
RocksDatabaseException {
- String sstFileName = fileToBeDeleted.fileName();
+ public void deleteFile(
+ ColumnFamilyHandle columnFamilyHandle,
+ LiveFileMetaData fileToBeDeleted) throws RocksDatabaseException {
Review Comment:
The `deleteFile` was deprecated in
https://github.com/facebook/rocksdb/pull/13284 and removed in
https://github.com/facebook/rocksdb/pull/13322.
As a result it should no longer be part of `ManagedRocksDB.java` either.
Any callers of the method should likely be migrated to use the existing
`deleteFilesInRanges` method.
--
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]
