Copilot commented on code in PR #9489: URL: https://github.com/apache/ozone/pull/9489#discussion_r2616053417
########## hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/ManagedSstFileIterator.java: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.utils.db; + +import org.apache.hadoop.hdds.utils.db.managed.ManagedOptions; +import org.apache.hadoop.hdds.utils.db.managed.ManagedReadOptions; +import org.apache.hadoop.hdds.utils.db.managed.ManagedSstFileReader; +import org.apache.hadoop.hdds.utils.db.managed.ManagedSstFileReaderIterator; +import org.apache.hadoop.ozone.util.ClosableIterator; +import org.rocksdb.RocksDBException; + +/** + * ManagedSstFileIterator is an abstract class designed to provide a managed, resource-safe + * iteration over SST (Sorted String Table) files leveraging RocksDB. It implements the + * {@link ClosableIterator} interface to support resource management and ensures proper + * cleanup of resources upon closure. This class binds together a ManagedSstFileReader, + * ManagedSstFileReaderIterator, and Buffers for keys and values, while allowing specific + * implementations to define how the iterator values are derived. + * + * @param <T> The type of the element to be returned by the iterator. + */ +abstract class ManagedSstFileIterator<T> implements ClosableIterator<T> { + private final ManagedSstFileReader fileReader; + private final ManagedSstFileReaderIterator fileReaderIterator; + private final IteratorType type; + private boolean closed; + private final Buffer keyBuffer; + private final Buffer valueBuffer; + + ManagedSstFileIterator(String path, ManagedOptions options, ManagedReadOptions readOptions, + IteratorType type) throws RocksDatabaseException { + try { + this.fileReader = new ManagedSstFileReader(options); + this.fileReader.open(path); + this.fileReaderIterator = ManagedSstFileReaderIterator.managed(fileReader.newIterator(readOptions)); + fileReaderIterator.get().seekToFirst(); + this.closed = false; + this.type = type; + this.keyBuffer = new Buffer( + new CodecBuffer.Capacity(path + " iterator-key", 1 << 10), + this.type.readKey() ? buffer -> fileReaderIterator.get().key(buffer) : null); + this.valueBuffer = new Buffer( + new CodecBuffer.Capacity(path + " iterator-value", 4 << 10), + this.type.readValue() ? buffer -> fileReaderIterator.get().value(buffer) : null); + } catch (RocksDBException e) { + throw new RocksDatabaseException("Failed to open SST file: " + path, e); + } + } + + @Override + public synchronized void close() { + if (!closed) { + this.fileReaderIterator.close(); + this.fileReader.close(); + keyBuffer.release(); + valueBuffer.release(); + } + closed = true; + } + + @Override + public boolean hasNext() { + return fileReaderIterator.get().isValid(); + } + + protected abstract T getIteratorValue(CodecBuffer key, CodecBuffer value); + + @Override + public T next() { + T value = getIteratorValue(this.type.readKey() ? keyBuffer.getFromDb() : null, + this.type.readValue() ? valueBuffer.getFromDb() : null); + fileReaderIterator.get().next(); + return value; Review Comment: The next() method is not synchronized while close() is. This creates a race condition where next() could be called while close() is executing, potentially reading from closed resources. For thread-safe operation, next() should also be synchronized or the implementation should document that it's not thread-safe. ########## hadoop-hdds/rocks-native/src/main/native/ManagedRawSSTFileIterator.cpp: ########## @@ -16,57 +16,50 @@ * limitations under the License. */ -#include "org_apache_hadoop_hdds_utils_db_managed_ManagedRawSSTFileIterator.h" +#include "org_apache_hadoop_hdds_utils_db_ManagedRawSSTFileIterator.h" #include "rocksdb/options.h" #include "rocksdb/raw_iterator.h" #include <string> #include "cplusplus_to_java_convert.h" #include <iostream> -jboolean Java_org_apache_hadoop_hdds_utils_db_managed_ManagedRawSSTFileIterator_hasNext(JNIEnv *env, jobject obj, +template <class T> +static jint copyToDirect(JNIEnv* env, T& source, jobject jtarget, jint jtarget_off, jint jtarget_len); + +jboolean Java_org_apache_hadoop_hdds_utils_db_ManagedRawSSTFileIterator_hasNext(JNIEnv *env, jobject obj, jlong native_handle) { return static_cast<jboolean>(reinterpret_cast<ROCKSDB_NAMESPACE::RawIterator*>(native_handle)->Valid()); } -void Java_org_apache_hadoop_hdds_utils_db_managed_ManagedRawSSTFileIterator_next(JNIEnv *env, jobject obj, +void Java_org_apache_hadoop_hdds_utils_db_ManagedRawSSTFileIterator_next(JNIEnv *env, jobject obj, jlong native_handle) { reinterpret_cast<ROCKSDB_NAMESPACE::RawIterator*>(native_handle)->Next(); } -jbyteArray Java_org_apache_hadoop_hdds_utils_db_managed_ManagedRawSSTFileIterator_getKey(JNIEnv *env, - jobject obj, - jlong native_handle) { +jint Java_org_apache_hadoop_hdds_utils_db_ManagedRawSSTFileIterator_getKey(JNIEnv *env, + jobject obj, + jlong native_handle, + jobject jtarget, + jint jtarget_off, jint jtarget_len) { ROCKSDB_NAMESPACE::Slice slice = reinterpret_cast<ROCKSDB_NAMESPACE::RawIterator*>(native_handle)->key(); - jbyteArray jkey = env->NewByteArray(static_cast<jsize>(slice.size())); - if (jkey == nullptr) { - // exception thrown: OutOfMemoryError - return nullptr; - } - env->SetByteArrayRegion( - jkey, 0, static_cast<jsize>(slice.size()), - const_cast<jbyte*>(reinterpret_cast<const jbyte*>(slice.data()))); - return jkey; + return copyToDirect(env, slice, jtarget, + jtarget_off, jtarget_len); } -jbyteArray Java_org_apache_hadoop_hdds_utils_db_managed_ManagedRawSSTFileIterator_getValue(JNIEnv *env, - jobject obj, - jlong native_handle) { +jint Java_org_apache_hadoop_hdds_utils_db_ManagedRawSSTFileIterator_getValue(JNIEnv *env, + jobject obj, + jlong native_handle, + jobject jtarget, + jint jtarget_off, jint jtarget_len) { ROCKSDB_NAMESPACE::Slice slice = reinterpret_cast<ROCKSDB_NAMESPACE::RawIterator*>(native_handle)->value(); jbyteArray jkey = env->NewByteArray(static_cast<jsize>(slice.size())); Review Comment: Dead code: This line creates a jbyteArray that is never used and immediately calls copyToDirect. This appears to be leftover code from the refactoring that should be removed. ```suggestion ``` ########## hadoop-hdds/rocks-native/src/main/native/ManagedRawSSTFileIterator.cpp: ########## @@ -75,16 +68,38 @@ jlong Java_org_apache_hadoop_hdds_utils_db_managed_ManagedRawSSTFileIterator_get } -jint Java_org_apache_hadoop_hdds_utils_db_managed_ManagedRawSSTFileIterator_getType(JNIEnv *env, +jint Java_org_apache_hadoop_hdds_utils_db_ManagedRawSSTFileIterator_getType(JNIEnv *env, jobject obj, jlong native_handle) { uint32_t type = reinterpret_cast<ROCKSDB_NAMESPACE::RawIterator*>(native_handle)->type(); return static_cast<jint>(type); } -void Java_org_apache_hadoop_hdds_utils_db_managed_ManagedRawSSTFileIterator_closeInternal(JNIEnv *env, +void Java_org_apache_hadoop_hdds_utils_db_ManagedRawSSTFileIterator_closeInternal(JNIEnv *env, jobject obj, jlong native_handle) { delete reinterpret_cast<ROCKSDB_NAMESPACE::RawIterator*>(native_handle); } + +template <class T> +static jint copyToDirect(JNIEnv* env, T& source, jobject jtarget, + jint jtarget_off, jint jtarget_len) { + char* target = reinterpret_cast<char*>(env->GetDirectBufferAddress(jtarget)); + if (target == nullptr || env->GetDirectBufferCapacity(jtarget) < (jtarget_off + jtarget_len)) { + jclass exClass = env->FindClass("java/lang/IllegalArgumentException"); + if (exClass == nullptr) { Review Comment: The error handling logic is incorrect. When FindClass returns nullptr (which means an exception is already thrown or the class cannot be found), the code checks `if (exClass == nullptr)` and then tries to call ThrowNew on a null pointer, which is undefined behavior. The condition should be `if (exClass != nullptr)` to only throw when the class was successfully found. ```suggestion if (exClass != nullptr) { ``` ########## hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/ManagedSstFileIterator.java: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.utils.db; + +import org.apache.hadoop.hdds.utils.db.managed.ManagedOptions; +import org.apache.hadoop.hdds.utils.db.managed.ManagedReadOptions; +import org.apache.hadoop.hdds.utils.db.managed.ManagedSstFileReader; +import org.apache.hadoop.hdds.utils.db.managed.ManagedSstFileReaderIterator; +import org.apache.hadoop.ozone.util.ClosableIterator; +import org.rocksdb.RocksDBException; + +/** + * ManagedSstFileIterator is an abstract class designed to provide a managed, resource-safe + * iteration over SST (Sorted String Table) files leveraging RocksDB. It implements the + * {@link ClosableIterator} interface to support resource management and ensures proper + * cleanup of resources upon closure. This class binds together a ManagedSstFileReader, + * ManagedSstFileReaderIterator, and Buffers for keys and values, while allowing specific + * implementations to define how the iterator values are derived. + * + * @param <T> The type of the element to be returned by the iterator. + */ +abstract class ManagedSstFileIterator<T> implements ClosableIterator<T> { + private final ManagedSstFileReader fileReader; + private final ManagedSstFileReaderIterator fileReaderIterator; + private final IteratorType type; + private boolean closed; + private final Buffer keyBuffer; + private final Buffer valueBuffer; + + ManagedSstFileIterator(String path, ManagedOptions options, ManagedReadOptions readOptions, + IteratorType type) throws RocksDatabaseException { + try { + this.fileReader = new ManagedSstFileReader(options); + this.fileReader.open(path); + this.fileReaderIterator = ManagedSstFileReaderIterator.managed(fileReader.newIterator(readOptions)); + fileReaderIterator.get().seekToFirst(); + this.closed = false; + this.type = type; + this.keyBuffer = new Buffer( + new CodecBuffer.Capacity(path + " iterator-key", 1 << 10), + this.type.readKey() ? buffer -> fileReaderIterator.get().key(buffer) : null); + this.valueBuffer = new Buffer( + new CodecBuffer.Capacity(path + " iterator-value", 4 << 10), + this.type.readValue() ? buffer -> fileReaderIterator.get().value(buffer) : null); + } catch (RocksDBException e) { + throw new RocksDatabaseException("Failed to open SST file: " + path, e); + } + } + + @Override + public synchronized void close() { + if (!closed) { + this.fileReaderIterator.close(); + this.fileReader.close(); + keyBuffer.release(); + valueBuffer.release(); + } + closed = true; + } + + @Override + public boolean hasNext() { + return fileReaderIterator.get().isValid(); + } + + protected abstract T getIteratorValue(CodecBuffer key, CodecBuffer value); + + @Override + public T next() { Review Comment: The hasNext() method is not synchronized while close() is. This creates a race condition where hasNext() could be called while close() is executing, potentially reading from closed resources. For thread-safe operation, hasNext() and next() should also be synchronized or the implementation should document that it's not thread-safe. ```suggestion public synchronized boolean hasNext() { return fileReaderIterator.get().isValid(); } protected abstract T getIteratorValue(CodecBuffer key, CodecBuffer value); @Override public synchronized T next() { ``` -- 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]
