ibessonov commented on code in PR #4679:
URL: https://github.com/apache/ignite-3/pull/4679#discussion_r1830486890
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/MetaStorageListener.java:
##########
@@ -225,6 +225,7 @@ public void onConfigurationCommitted(CommittedConfiguration
config) {
@Override
public void onSnapshotSave(Path path, Consumer<Throwable> doneClo) {
storage.snapshot(path)
+ .thenCompose(unused -> storage.flush())
Review Comment:
I think that the storage should call `flush` inside of `snapshot`, we
shouldn't manually call them in conjunction
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -831,19 +833,13 @@ public class IgniteImpl implements Ignite {
GcConfiguration gcConfig =
clusterConfigRegistry.getConfiguration(GcExtensionConfiguration.KEY).gc();
- LogSyncer logSyncer = () -> {
- partitionsLogStorageFactory.sync();
- cmgLogStorageFactory.sync();
- msLogStorageFactory.sync();
- };
-
Map<String, StorageEngine> storageEngines =
dataStorageModules.createStorageEngines(
name,
nodeConfigRegistry,
storagePath,
longJvmPauseDetector,
failureManager,
- logSyncer,
+ partitionsUseFsyncFroWriteToRaftLog ?
partitionsLogStorageFactory : new NoOpLogSyncer(),
Review Comment:
There's no need to have this condition. Log factory itself will do a no-op
if fsync is disabled. This code doesn't need to be more complicated than it was
##########
modules/rocksdb-common/src/main/java/org/apache/ignite/internal/rocksdb/flush/RocksDbFlusher.java:
##########
@@ -279,6 +281,18 @@ public void stop() {
* @return Future that completes when the {@code onFlushCompleted}
callback finishes.
*/
CompletableFuture<Void> onFlushCompleted() {
- return CompletableFuture.runAsync(onFlushCompleted, threadPool);
+ return inBusyLockSafeAsync(() -> runAsync(onFlushCompleted,
threadPool));
+ }
+
+ private CompletableFuture<Void>
inBusyLockSafeAsync(Supplier<CompletableFuture<Void>> supplier) {
Review Comment:
No, it is not a normal convention. We decided to give up because we couldn't
come up with a better name. According to Ignite's conventions, and generally
accepted conventions in Java, `async` methods do something asynchronously,
which is not the case here
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1011,7 +1007,7 @@ public class IgniteImpl implements Ignite {
lowWatermark,
transactionInflights,
indexMetaStorage,
- logSyncer,
+ partitionsUseFsyncFroWriteToRaftLog ?
partitionsLogStorageFactory : new NoOpLogSyncer(),
Review Comment:
Same here
--
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]
