ibessonov commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021268903
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/io/PartitionMetaIo.java:
##########
@@ -83,6 +88,30 @@ public void setLastAppliedIndex(long pageAddr, long
lastAppliedIndex) {
putLong(pageAddr, LAST_APPLIED_INDEX_OFF, lastAppliedIndex);
}
+ /**
+ * Sets a last applied term value.
+ *
+ * @param pageAddr Page address.
+ * @param lastAppliedTerm Last applied term value.
+ */
+ public void setLastAppliedTerm(long pageAddr, long lastAppliedTerm) {
+ assertPageType(pageAddr);
+
+ putLong(pageAddr, LAST_APPLIED_TERM_OFF, lastAppliedTerm);
+ }
+
+ /**
+ * Sets last group config.
+ *
+ * @param pageAddr Page address.
+ * @param lastGroupConfig Byte representation of last group config..
+ */
+ public void setLastGroupConfig(long pageAddr, byte @Nullable []
lastGroupConfig) {
+ assertPageType(pageAddr);
+
+ // TODO: IGNITE-18118 - implement
Review Comment:
Ok, I see now, there's no implementation yet.
The issue lacks description. Please describe everything you're going to do,
I need to see a design for the implementation.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionSnapshotStorage.java:
##########
@@ -182,12 +185,19 @@ public SnapshotWriter create() {
}
@Override
+ @Nullable
public SnapshotReader open() {
if (startupSnapshotOpened.compareAndSet(false, true)) {
+ if (startupSnapshotMeta == null) {
Review Comment:
Never thought about it, good catch!
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PartitionMeta.java:
##########
@@ -341,6 +399,7 @@ public int pageCount() {
*/
void writeTo(PartitionMetaIo metaIo, long pageAddr) {
metaIo.setLastAppliedIndex(pageAddr, lastAppliedIndex);
+ metaIo.setLastAppliedTerm(pageAddr, lastAppliedTerm);
Review Comment:
What if it doesn't fit? Do we have a hard limit?
This part should be redesigned, I don't trust it. We should probably have
the array as a "transient" field and a link that will be stored to the storage.
Of course, array should be put into a free list in this case.
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -178,11 +180,29 @@ public class RocksDbMvPartitionStorage implements
MvPartitionStorage {
/** Key to store applied index value in meta. */
private final byte[] lastAppliedIndexKey;
+ /** Key to store applied term value in meta. */
+ private final byte[] lastAppliedTermKey;
Review Comment:
What if we would join these two into a single key-value pair? They're never
updated separately
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/FSMCallerImpl.java:
##########
@@ -665,6 +676,27 @@ private void doSnapshotLoad(final LoadSnapshotClosure
done) {
setError(e);
return;
}
+
+ // JRaft tests use such strange metas, so we have to protect... in
production, these are never null.
+ if (meta.peersList() != null && meta.learnersList() != null) {
Review Comment:
Can a list of learners be null? I don't quite understand why we check both
lists here. Maybe you should expand the comment a little bit, wouldn't hurt.
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/FSMCallerImpl.java:
##########
@@ -665,6 +676,27 @@ private void doSnapshotLoad(final LoadSnapshotClosure
done) {
setError(e);
return;
}
+
+ // JRaft tests use such strange metas, so we have to protect... in
production, these are never null.
+ if (meta.peersList() != null && meta.learnersList() != null) {
+ ConfigurationEntry configurationEntry = new ConfigurationEntry(
+ snapshotId.copy(),
+ new Configuration(
+
meta.peersList().stream().map(PeerId::parsePeer).collect(toList()),
+
meta.learnersList().stream().map(PeerId::parsePeer).collect(toList())
+ ),
+ new Configuration()
+ );
+ if (meta.oldPeersList() != null && !meta.oldPeersList().isEmpty())
{
+ configurationEntry.setOldConf(new Configuration(
+
meta.oldPeersList().stream().map(PeerId::parsePeer).collect(toList()),
Review Comment:
Just a question. I remember @sashapolo having a huge PR where he did stuff
with peer ids. Is it in your branch?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/snapshot/SnapshotExecutorImpl.java:
##########
@@ -217,7 +217,7 @@ public boolean init(final SnapshotExecutorOptions opts) {
this.term = opts.getInitTerm();
this.msgFactory = node.getRaftOptions().getRaftMessagesFactory();
this.snapshotStorage =
this.node.getServiceFactory().createSnapshotStorage(opts.getUri(),
- this.node.getRaftOptions(), logManager);
+ this.node.getRaftOptions() );
Review Comment:
```suggestion
this.node.getRaftOptions());
```
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ItTablePersistenceTest.java:
##########
@@ -213,6 +230,28 @@ public BooleanSupplier
snapshotCheckClosure(JraftServerImpl restarted, boolean i
};
}
+ private static Map<ByteBuffer, RowId> rowsToRowIds(MvPartitionStorage
storage) {
+ Map<ByteBuffer, RowId> result = new HashMap<>();
+
+ RowId rowId = storage.closestRowId(RowId.lowestRowId(0));
+
+ while (rowId != null) {
Review Comment:
At one point, we'll make an abstraction that encapsulates this loop. A
Cursor, maybe. These are just thoughts
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -327,15 +383,23 @@ public void onSnapshotSave(Path path, Consumer<Throwable>
doneClo) {
// 4) When we try to restore data starting from the minimal
lastAppliedIndex, we come to the situation
// that a raft node doesn't have such data, because the
truncation until the maximal lastAppliedIndex from 1) has happened.
// 5) Node cannot finish local recovery.
- long maxLastAppliedIndex = Math.max(storage.lastAppliedIndex(),
txStateStorage.lastAppliedIndex());
+ long maxLastAppliedIndex;
+ long maxLastAppliedTerm;
Review Comment:
Can we calculate term as a max of two terms? This would simplify the code,
but only if it's a correct replacement,
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotReader.java:
##########
@@ -66,7 +66,7 @@ public void shutdown() {
@Override
public String getPath() {
- throw new UnsupportedOperationException("No path for the rebalance
snapshot");
+ return "";
Review Comment:
How's this required? Am I missing something?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/outgoing/OutgoingSnapshot.java:
##########
@@ -143,24 +141,36 @@ public PartitionKey partitionKey() {
/**
* Freezes the scope of this snapshot. This includes taking snapshot
metadata and opening TX data cursor.
- *
- * <p>Must be called under snapshot lock.
*/
- void freezeScope() {
- assert mvOperationsLock.isLocked() : "MV operations lock must be
acquired!";
+ void freezeScopeUnderMvLock() {
+ acquireMvLock();
- frozenMeta = takeSnapshotMeta();
+ try {
+ frozenMeta = takeSnapshotMeta();
- txDataCursor = partition.txStatePartitionStorage().scan();
+ txDataCursor = partition.txStatePartitionStorage().scan();
+ } finally {
+ releaseMvLock();
+ }
}
private SnapshotMeta takeSnapshotMeta() {
- long lastAppliedIndex = Math.max(
- partition.mvPartitionStorage().lastAppliedIndex(),
- partition.txStatePartitionStorage().lastAppliedIndex()
- );
+ long lastAppliedIndex;
+ long lastAppliedTerm;
- return SnapshotMetaUtils.snapshotMetaAt(lastAppliedIndex, logManager);
+ if (partition.mvPartitionStorage().lastAppliedIndex() >=
partition.txStatePartitionStorage().lastAppliedIndex()) {
+ lastAppliedIndex =
partition.mvPartitionStorage().lastAppliedIndex();
+ lastAppliedTerm = partition.mvPartitionStorage().lastAppliedTerm();
+ } else {
+ lastAppliedIndex =
partition.txStatePartitionStorage().lastAppliedIndex();
+ lastAppliedTerm =
partition.txStatePartitionStorage().lastAppliedTerm();
+ }
+
+ return SnapshotMetaUtils.snapshotMetaAt(
+ lastAppliedIndex,
+ lastAppliedTerm,
+
requireNonNull(partition.mvPartitionStorage().committedGroupConfiguration())
Review Comment:
But what if it is null? Is NPE an expected behavior?
--
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]
