Phillippko commented on code in PR #4200:
URL: https://github.com/apache/ignite-3/pull/4200#discussion_r1718385001
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/instance/SharedRocksDbInstance.java:
##########
@@ -119,6 +119,8 @@ public void close() {
/** Column Family for GC queue. */
public final ColumnFamily gcQueueCf;
+ public final ColumnFamily dataCf;
Review Comment:
Let's add javadoc
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java:
##########
@@ -136,6 +136,10 @@ ColumnFamilyHandle gcQueueHandle() {
return rocksDb.gcQueueCf.handle();
}
+ ColumnFamilyHandle dataCfHandle() {
Review Comment:
javadoc
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -116,9 +136,17 @@
*/
public class RocksDbMvPartitionStorage implements MvPartitionStorage {
/** Thread-local on-heap byte buffer instance to use for key
manipulations. */
- private static final ThreadLocal<ByteBuffer> HEAP_KEY_BUFFER =
withInitial(() -> allocate(MAX_KEY_SIZE).order(KEY_BYTE_ORDER));
+ private static final ThreadLocal<ByteBuffer>
HEAP_COMMITTED_DATA_ID_KEY_BUFFER =
+ withInitial(() -> allocate(MAX_KEY_SIZE).order(KEY_BYTE_ORDER));
- private static final ThreadLocal<ByteBuffer> DIRECT_KEY_BUFFER =
withInitial(() -> allocateDirect(MAX_KEY_SIZE).order(KEY_BYTE_ORDER));
+ private static final ThreadLocal<ByteBuffer> HEAP_DATA_ID_KEY_BUFFER =
+ withInitial(() -> allocate(ROW_PREFIX_SIZE).order(KEY_BYTE_ORDER));
+
+ private static final ThreadLocal<ByteBuffer> DIRECT_DATA_ID_KEY_BUFFER =
Review Comment:
we have exactly the same buffers here and in GarbageCollector, is it
intentional? We can't reuse them?
And in GC we have javadocs for buffers, let's add javadocs here?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/PartitionDataHelper.java:
##########
@@ -263,6 +294,59 @@ static BinaryRow deserializeRow(ByteBuffer buffer) {
return new BinaryRowImpl(schemaVersion, binaryTupleSlice);
}
+ byte[] createPayloadKey(ByteBuffer dataId) {
+ byte[] result = PAYLOAD_KEY_BUFFER.get()
+ .clear()
+ .putInt(tableId)
+ .putShort((short) partitionId)
+ .put(dataId)
+ .array();
+
+ dataId.rewind();
+
+ // Always use 0 for the last bit (tombstone flag), because it only
makes sense when data ID is stored as a value.
+ setLastBit(result, result.length - 1, false);
+
+ return result;
+ }
+
+ /**
+ * Changes the last bit of the byte identified by an index in an array.
+ *
+ * @param array Array containing the byte to change.
+ * @param index Index of the byte inside the array.
+ * @param value If {@code true} - sets the bit to 1, else to 0.
+ */
+ static void setLastBit(byte[] array, int index, boolean value) {
+ byte lastByte = array[index];
+
+ if (value) {
+ lastByte |= 0x01;
+ } else {
+ lastByte &= 0xFE;
+ }
+
+ array[index] = lastByte;
Review Comment:
```suggestion
if (value) {
array[index] |= 0x01;
} else {
array[index] &= 0xFE;
}
```
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/ColumnFamilyUtils.java:
##########
@@ -42,12 +42,15 @@ public class ColumnFamilyUtils {
/** Name of the meta column family matches default columns family, meaning
that it always exist when new table is created. */
private static final String META_CF_NAME = "default";
- /** Name of the Column Family that stores partition data. */
+ /** Name of the Column Family that stores partition data with references
to row data. */
private static final String PARTITION_CF_NAME = "cf-part";
/** Name of the Column Family that stores garbage collection queue. */
private static final String GC_QUEUE_CF_NAME = "cf-gc";
+ /** Name of the Column Family that stores row data. */
Review Comment:
Let's move it closer to cf-part, before cf-gc
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbStorageEngine.java:
##########
@@ -152,6 +152,7 @@ public String name() {
@Override
public void start() throws StorageException {
// TODO: IGNITE-17066 Add handling deleting/updating storage profiles
configuration
+ storageConfiguration.profiles().value();
Review Comment:
leftovers
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/GarbageCollector.java:
##########
@@ -73,8 +76,17 @@ class GarbageCollector {
/** Garbage collector's queue key's size. */
private static final int GC_KEY_SIZE = GC_KEY_ROW_ID_OFFSET + ROW_ID_SIZE;
- /** Thread-local direct buffer instance to read keys from RocksDB. */
- private static final ThreadLocal<ByteBuffer> GC_KEY_BUFFER =
withInitial(() -> allocateDirect(GC_KEY_SIZE).order(KEY_BYTE_ORDER));
+ /** Thread-local direct buffer instance to read keys that contain Data ID.
*/
+ private static final ThreadLocal<ByteBuffer> DIRECT_DATA_ID_KEY_BUFFER =
+ withInitial(() ->
allocateDirect(MAX_KEY_SIZE).order(KEY_BYTE_ORDER));
+
+ /** Thread-local direct buffer for storing Data ID. */
Review Comment:
```suggestion
/** Thread-local direct buffer for Data ID manipulations. */
```
Also, let's fix descriptions for other buffers? They're not only "to read"
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/ColumnFamilyUtils.java:
##########
@@ -85,6 +89,8 @@ public static ColumnFamilyType fromCfName(String cfName) {
return PARTITION;
} else if (GC_QUEUE_CF_NAME.equals(cfName)) {
return GC_QUEUE;
+ } else if (DATA_CF_NAME.equals(cfName)) {
Review Comment:
I think it's time to use switch
if (cfName.startsWith(SORTED_INDEX_CF_PREFIX)) {
return SORTED_INDEX;
}
switch (cfName) {
case META_CF_NAME:
return META;
case PARTITION_CF_NAME:
return PARTITION;
case GC_QUEUE_CF_NAME:
return GC_QUEUE;
case DATA_CF_NAME:
return DATA;
case HASH_INDEX_CF_NAME:
return HASH_INDEX;
default:
return UNKNOWN;
}
--
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]
