tkalkirill commented on code in PR #1505:
URL: https://github.com/apache/ignite-3/pull/1505#discussion_r1065478019
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/StorageException.java:
##########
@@ -17,17 +17,23 @@
package org.apache.ignite.internal.storage;
+import org.apache.ignite.lang.ErrorGroups.Storage;
+import org.apache.ignite.lang.IgniteInternalException;
+
/**
* Exception thrown by the storage.
*/
-public class StorageException extends RuntimeException {
+public class StorageException extends IgniteInternalException {
+ /** Serial version uid. */
Review Comment:
```suggestion
```
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/StorageException.java:
##########
@@ -17,17 +17,23 @@
package org.apache.ignite.internal.storage;
+import org.apache.ignite.lang.ErrorGroups.Storage;
+import org.apache.ignite.lang.IgniteInternalException;
+
/**
* Exception thrown by the storage.
*/
-public class StorageException extends RuntimeException {
+public class StorageException extends IgniteInternalException {
+ /** Serial version uid. */
+ private static final long serialVersionUID = 8705275268121031742L;
Review Comment:
Why do we need `serialVersionUID`?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/SortedIndex.java:
##########
@@ -70,6 +71,20 @@ void destroy() {
}
}
+ /**
+ * Deletes the data associated with the partition in the index, using
passed write batch for the operation.
+ *
+ * @throws RocksDBException If failed to delete data.
+ */
+ void destroy(int partitionId, WriteBatch writeBatch) throws
RocksDBException {
Review Comment:
Please point out that we don't just delete the contents of the index, but
also close it if it exists.
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/HashIndex.java:
##########
@@ -55,4 +57,18 @@ HashIndexStorage
getOrCreateStorage(RocksDbMvPartitionStorage partitionStorage)
void destroy() {
storages.forEach((partitionId, storage) -> storage.destroy());
}
+
+ /**
+ * Deletes the data associated with the partition in the index, using
passed write batch for the operation.
+ *
+ * @throws RocksDBException If failed to delete data.
+ */
+ void destroy(int partitionId, WriteBatch writeBatch) throws
RocksDBException {
+ RocksDbHashIndexStorage hashIndex = storages.remove(partitionId);
+
+ if (hashIndex != null) {
+ hashIndex.close();
+ hashIndex.destroyData(writeBatch);
Review Comment:
```suggestion
hashIndex.close();
hashIndex.destroyData(writeBatch);
```
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/StorageClosedException.java:
##########
@@ -17,35 +17,19 @@
package org.apache.ignite.internal.storage;
+import org.apache.ignite.lang.ErrorGroups.Storage;
+
/**
* Exception that will be thrown when the storage is closed.
*/
public class StorageClosedException extends StorageException {
- /**
- * Constructor.
- *
- * @param message Error message.
- */
- public StorageClosedException(String message) {
- super(message);
- }
-
- /**
- * Constructor.
- *
- * @param message Error message.
- * @param cause The cause.
- */
- public StorageClosedException(String message, Throwable cause) {
- super(message, cause);
- }
+ /** Serial version uid. */
+ private static final long serialVersionUID = -7988332521347221109L;
Review Comment:
Why do we need `serialVersionUID`?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/SortedIndex.java:
##########
@@ -70,6 +71,20 @@ void destroy() {
}
}
+ /**
+ * Deletes the data associated with the partition in the index, using
passed write batch for the operation.
+ *
+ * @throws RocksDBException If failed to delete data.
+ */
+ void destroy(int partitionId, WriteBatch writeBatch) throws
RocksDBException {
+ RocksDbSortedIndexStorage sortedIndex = storages.remove(partitionId);
+
+ if (sortedIndex != null) {
+ sortedIndex.close();
+ sortedIndex.destroyData(writeBatch);
Review Comment:
```suggestion
sortedIndex.close();
sortedIndex.destroyData(writeBatch);
```
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/HashIndex.java:
##########
@@ -55,4 +57,18 @@ HashIndexStorage
getOrCreateStorage(RocksDbMvPartitionStorage partitionStorage)
void destroy() {
storages.forEach((partitionId, storage) -> storage.destroy());
}
+
+ /**
+ * Deletes the data associated with the partition in the index, using
passed write batch for the operation.
+ *
+ * @throws RocksDBException If failed to delete data.
+ */
+ void destroy(int partitionId, WriteBatch writeBatch) throws
RocksDBException {
Review Comment:
Please point out that we don't just delete the contents of the index, but
also close it if it exists.
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/StorageException.java:
##########
@@ -46,6 +52,10 @@ public StorageException(String message, Throwable cause) {
* @param cause The cause.
*/
public StorageException(Throwable cause) {
- super(cause);
+ super(Storage.GENERIC_ERR, cause);
+ }
+
+ protected StorageException(int code, String message) {
Review Comment:
Missing description.
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/StorageClosedException.java:
##########
@@ -17,35 +17,19 @@
package org.apache.ignite.internal.storage;
+import org.apache.ignite.lang.ErrorGroups.Storage;
+
/**
* Exception that will be thrown when the storage is closed.
*/
public class StorageClosedException extends StorageException {
- /**
- * Constructor.
- *
- * @param message Error message.
- */
- public StorageClosedException(String message) {
- super(message);
- }
-
- /**
- * Constructor.
- *
- * @param message Error message.
- * @param cause The cause.
- */
- public StorageClosedException(String message, Throwable cause) {
- super(message, cause);
- }
+ /** Serial version uid. */
Review Comment:
```suggestion
```
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -134,14 +137,14 @@ public class RocksDbMvPartitionStorage implements
MvPartitionStorage {
/** Thread-local direct buffer instance to read keys from RocksDB. */
private static final ThreadLocal<ByteBuffer> MV_KEY_BUFFER =
withInitial(() -> allocateDirect(MAX_KEY_SIZE).order(KEY_BYTE_ORDER));
- /** Thread-local write batch for {@link #runConsistently(WriteClosure)}. */
- private static final ThreadLocal<WriteBatchWithIndex> WRITE_BATCH = new
ThreadLocal<>();
-
/** Thread-local on-heap byte buffer instance to use for key
manipulations. */
private static final ThreadLocal<ByteBuffer> HEAP_KEY_BUFFER = withInitial(
() -> ByteBuffer.allocate(MAX_KEY_SIZE).order(KEY_BYTE_ORDER)
);
+ /** Thread-local write batch for {@link #runConsistently(WriteClosure)}. */
+ private final ThreadLocal<WriteBatchWithIndex> threadLocalWriteBatch = new
ThreadLocal<>();
Review Comment:
Why not `static` ?
--
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]
