tkalkirill commented on code in PR #1619:
URL: https://github.com/apache/ignite-3/pull/1619#discussion_r1095544350
##########
gradle/libs.versions.toml:
##########
@@ -57,7 +57,7 @@ jsonpath = "2.4.0"
classgraph = "4.8.110"
javassist = "3.28.0-GA"
checker = "3.10.0"
-rocksdb = "7.3.1"
+rocksdb = "7.9.2"
Review Comment:
Why did you decide to update the version?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java:
##########
@@ -225,8 +225,9 @@ public interface MvPartitionStorage extends
ManuallyCloseable {
* @param lowWatermark A time threshold for the row. Rows younger then the
watermark value will not be removed.
* @return A pair of table row and row id, where a timestamp of the row is
less than or equal to {@code lowWatermark}.
* {@code null} if there's no such value.
+ * @throws StorageException If failed to poll element for vacuum.
*/
- default @Nullable TableRowAndRowId pollForVacuum(HybridTimestamp
lowWatermark) {
+ default @Nullable TableRowAndRowId pollForVacuum(HybridTimestamp
lowWatermark) throws StorageException {
Review Comment:
`StorageException` is not checked, there is no need to write it in the
signature, it is enough to mention it in the documentation.
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/BaseMvStoragesTest.java:
##########
@@ -52,58 +52,36 @@
import org.apache.ignite.lang.IgniteBiTuple;
import org.apache.ignite.lang.IgniteException;
import org.jetbrains.annotations.Nullable;
-import org.junit.jupiter.api.AfterAll;
-import org.junit.jupiter.api.BeforeAll;
/**
* Base test for MV storages, contains pojo classes, their descriptor and a
marshaller instance.
*/
public abstract class BaseMvStoragesTest {
/** Default reflection marshaller factory. */
- protected static MarshallerFactory marshallerFactory;
+ protected static final MarshallerFactory marshallerFactory = new
ReflectionMarshallerFactory();
/** Schema descriptor for tests. */
- protected static SchemaDescriptor schemaDescriptor;
+ protected static final SchemaDescriptor schemaDescriptor = new
SchemaDescriptor(1, new Column[]{
+ new Column("intKey".toUpperCase(Locale.ROOT), NativeTypes.INT32,
false),
Review Comment:
Why do you need **.toUpperCase(Locale.ROOT)**?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java:
##########
@@ -586,7 +602,7 @@ private List<String> getExistingCfNames() {
// even if the database is new (no existing Column Families), we
return the names of mandatory column families, that
// will be created automatically.
- return existingNames.isEmpty() ? List.of(META_CF_NAME,
PARTITION_CF_NAME, HASH_INDEX_CF_NAME) : existingNames;
+ return existingNames.isEmpty() ? List.of(META_CF_NAME,
PARTITION_CF_NAME, GC_QUEUE_CF_NAME, HASH_INDEX_CF_NAME) : existingNames;
Review Comment:
Just thoughts: It would be more convenient to iterate with an enum.
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/ColumnFamilyUtils.java:
##########
@@ -35,6 +35,11 @@ class ColumnFamilyUtils {
*/
static final String PARTITION_CF_NAME = "cf-part";
+ /**
+ * Name of the Column Family that stores garbage collection queue.
+ */
Review Comment:
```suggestion
/** Name of the Column Family that stores garbage collection queue. */
```
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/ColumnFamilyUtils.java:
##########
@@ -49,7 +54,7 @@ class ColumnFamilyUtils {
* Utility enum to describe a type of the column family - meta or
partition.
*/
enum ColumnFamilyType {
- META, PARTITION, HASH_INDEX, SORTED_INDEX, UNKNOWN;
+ META, PARTITION, GC_QUEUE, HASH_INDEX, SORTED_INDEX, UNKNOWN;
Review Comment:
Just thoughts: why not write down the names in the enumeration, for example,
`GC_QUEUE("cf-gc")`, or use the enum name as the name for the cf.
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/BaseMvStoragesTest.java:
##########
@@ -181,7 +159,7 @@ protected static List<IgniteBiTuple<TestKey, TestValue>>
drainToList(Cursor<Read
}
}
- protected final void assertRowMatches(TableRow rowUnderQuestion, TableRow
expectedRow) {
+ protected final void assertRowMatches(@Nullable TableRow rowUnderQuestion,
TableRow expectedRow) {
Review Comment:
Why is **rowUnderQuestion** `@Nullable`? Logically, it just can't be.
--
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]
