You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/08/09 15:33:59 UTC
[GitHub] [ignite-3] sashapolo opened a new pull request, #994: IGNITE-17500 Fix minor issues in RocksDbMvPartitionStorage and RocksDbTableStorage
sashapolo opened a new pull request, #994:
URL: https://github.com/apache/ignite-3/pull/994
* "Destroy" method returns a future that completes after the data is flushed
* Storage is closed after "destroy" is called
* FlushOptions are cached in RocksDbTableStorage so that they do not get GC-ed
*
https://issues.apache.org/jira/browse/IGNITE-17500
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #994: IGNITE-17500 Fix minor issues in RocksDB-based partition storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #994:
URL: https://github.com/apache/ignite-3/pull/994#discussion_r941538391
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -193,11 +193,9 @@ public <V> V runConsistently(WriteClosure<V> closure) throws StorageException {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> flush() {
- CompletableFuture<Void> flushFuture = new CompletableFuture<>();
-
- CompletableFuture<Void> oldFuture = flushFuturesByAppliedIndex.put(lastAppliedIndex, flushFuture);
-
- assert oldFuture == null;
+ CompletableFuture<Void> flushFuture = flushFuturesByAppliedIndex.computeIfAbsent(
Review Comment:
Because `destroy` could have been called after a Raft command has been applied. This means that we will call `flush` twice: once for the command and once for the destroy, but they will have the same applied index. Instead of failing with the assertion, we should simply return the previous future
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #994: IGNITE-17500 Fix minor issues in RocksDB-based partition storages
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #994:
URL: https://github.com/apache/ignite-3/pull/994#discussion_r941540235
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -193,11 +193,9 @@ public <V> V runConsistently(WriteClosure<V> closure) throws StorageException {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> flush() {
- CompletableFuture<Void> flushFuture = new CompletableFuture<>();
-
- CompletableFuture<Void> oldFuture = flushFuturesByAppliedIndex.put(lastAppliedIndex, flushFuture);
-
- assert oldFuture == null;
+ CompletableFuture<Void> flushFuture = flushFuturesByAppliedIndex.computeIfAbsent(
Review Comment:
ok
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #994: IGNITE-17500 Fix minor issues in RocksDB-based partition storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #994:
URL: https://github.com/apache/ignite-3/pull/994#discussion_r941538391
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -193,11 +193,9 @@ public <V> V runConsistently(WriteClosure<V> closure) throws StorageException {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> flush() {
- CompletableFuture<Void> flushFuture = new CompletableFuture<>();
-
- CompletableFuture<Void> oldFuture = flushFuturesByAppliedIndex.put(lastAppliedIndex, flushFuture);
-
- assert oldFuture == null;
+ CompletableFuture<Void> flushFuture = flushFuturesByAppliedIndex.computeIfAbsent(
Review Comment:
Because `destroy` could have been called after a Raft command has been applied. This means that we will call `flush` twice: once for the command and once for the destroy, but they will have the same applied index
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #994: IGNITE-17500 Fix minor issues in RocksDB-based partition storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #994:
URL: https://github.com/apache/ignite-3/pull/994#discussion_r941529001
##########
modules/storage-rocksdb/src/test/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorageTest.java:
##########
@@ -246,7 +248,8 @@ void storageAdvertisesItIsPersistent() {
assertThat(storage.isVolatile(), is(false));
}
- private IgniteBiTuple<TestKey, TestValue> unwrap(BinaryRow binaryRow) {
+ @Nullable
+ private static IgniteBiTuple<TestKey, TestValue> unwrap(@Nullable BinaryRow binaryRow) {
Review Comment:
Why?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes merged pull request #994: IGNITE-17500 Fix minor issues in RocksDB-based partition storages
Posted by GitBox <gi...@apache.org>.
SammyVimes merged PR #994:
URL: https://github.com/apache/ignite-3/pull/994
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #994: IGNITE-17500 Fix minor issues in RocksDB-based partition storages
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #994:
URL: https://github.com/apache/ignite-3/pull/994#discussion_r941506118
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -193,11 +193,9 @@ public <V> V runConsistently(WriteClosure<V> closure) throws StorageException {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> flush() {
- CompletableFuture<Void> flushFuture = new CompletableFuture<>();
-
- CompletableFuture<Void> oldFuture = flushFuturesByAppliedIndex.put(lastAppliedIndex, flushFuture);
-
- assert oldFuture == null;
+ CompletableFuture<Void> flushFuture = flushFuturesByAppliedIndex.computeIfAbsent(
Review Comment:
Why did you remove the assert?
##########
modules/storage-rocksdb/src/test/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorageTest.java:
##########
@@ -246,7 +248,8 @@ void storageAdvertisesItIsPersistent() {
assertThat(storage.isVolatile(), is(false));
}
- private IgniteBiTuple<TestKey, TestValue> unwrap(BinaryRow binaryRow) {
+ @Nullable
+ private static IgniteBiTuple<TestKey, TestValue> unwrap(@Nullable BinaryRow binaryRow) {
Review Comment:
```suggestion
private static @Nullable IgniteBiTuple<TestKey, TestValue> unwrap(@Nullable BinaryRow binaryRow) {
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #994: IGNITE-17500 Fix minor issues in RocksDB-based partition storages
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #994:
URL: https://github.com/apache/ignite-3/pull/994#discussion_r941531844
##########
modules/storage-rocksdb/src/test/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorageTest.java:
##########
@@ -246,7 +248,8 @@ void storageAdvertisesItIsPersistent() {
assertThat(storage.isVolatile(), is(false));
}
- private IgniteBiTuple<TestKey, TestValue> unwrap(BinaryRow binaryRow) {
+ @Nullable
+ private static IgniteBiTuple<TestKey, TestValue> unwrap(@Nullable BinaryRow binaryRow) {
Review Comment:
This is just a suggestion: null can be a return value, not a modifier.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org