IGNITE-17894 Implement RAFT snapshot streaming receiver #1233
Conversation
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
Show resolved
Hide resolved
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/MvPartitionAccess.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/MvPartitionAccess.java
Outdated
Show resolved
Hide resolved
.../apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java
Outdated
Show resolved
Hide resolved
.../apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java
Outdated
Show resolved
Hide resolved
.../apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java
Outdated
Show resolved
Hide resolved
.../apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java
Outdated
Show resolved
Hide resolved
...che/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopierTest.java
Outdated
Show resolved
Hide resolved
...che/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopierTest.java
Outdated
Show resolved
Hide resolved
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
Show resolved
Hide resolved
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionAccess.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| return loadSnapshotMeta(snapshotSender) | ||
| .thenCompose(unused1 -> loadSnapshotMvData(snapshotSender, executor)) |
There was a problem hiding this comment.
Would these compositions look nicer with methods references? You wouldn't need to make names like "unused1" in that case, just move it to parameters list with the same name everywhere
There was a problem hiding this comment.
But this would be a useless argument, why do this? Maybe I didn't understand you.
There was a problem hiding this comment.
Both options looks weird, so it's up to you to decide.
Maybe you should use thenRun, it has no unused argument.
Another thing I noticed is that you always pass executor as argument into methods like "loadSnapshotMvData". Can't it be a field?
There was a problem hiding this comment.
I'll leave it as is.
thenRun does not return the future, but I need it so that the cancel works correctly with an interrupt.
I can even not use the field, it seems to me that it's a little clearer that we will execute the task in another thread.
There was a problem hiding this comment.
You're right, I forgot about it
There was a problem hiding this comment.
I can even not use the field
Why?
There was a problem hiding this comment.
Since it is in IncomingSnapshotCopier#partitionSnapshotStorage#incomingSnapshotsExecutor
| public void start() { | ||
| Executor executor = partitionSnapshotStorage.getIncomingSnapshotsExecutor(); | ||
|
|
||
| LOG.info("Copier is started for the partition: " + partId()); |
There was a problem hiding this comment.
First of all, what's the common pattern of passing arguments? I feel like it's using {}.
Second, I would prefer this one to be wrapped into a "if (infoEnabled)" section. But I'm not sure that we actually have this as a rule. I would really love it if it was a rule
There was a problem hiding this comment.
Corrected the format, do we need if (infoEnabled) if there is a check inside?
There was a problem hiding this comment.
I vividly remember this topic being discussed a long time ago. The point is that the call without check creates a vararg array argument. It doesn't affect anything in most cases, but sometimes it may become a problem. I didn't invent this, this is how we used to write logs in Ignite 2
There was a problem hiding this comment.
I agree with you, but we don't have rules for 3.0 yet, maybe we should discuss it and add it to the code style rules.
There was a problem hiding this comment.
Then, after discussion, a new approach should be applied everywhere.
.../apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java
Show resolved
Hide resolved
.../apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java
Outdated
Show resolved
Hide resolved
.../apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java
Outdated
Show resolved
Hide resolved
.../apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/ignite/internal/tx/storage/state/rocksdb/TxStateRocksDbStorage.java
Outdated
Show resolved
Hide resolved
https://issues.apache.org/jira/browse/IGNITE-17894