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/11/11 08:29:36 UTC
[GitHub] [ignite-3] rpuch opened a new pull request, #1331: IGNITE-18122 Track last applied term and group config in storages
rpuch opened a new pull request, #1331:
URL: https://github.com/apache/ignite-3/pull/1331
https://issues.apache.org/jira/browse/IGNITE-18122
--
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] ibessonov commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021376699
##########
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:
No, nothing's wrong here, I'm worrying about conflicts, that's it
--
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 #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020042717
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaTest.java:
##########
@@ -41,15 +44,45 @@ void testLastAppliedIndex() {
assertEquals(0, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(null, 100));
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
assertEquals(100, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(UUID.randomUUID(), 500));
+ assertDoesNotThrow(() -> meta.lastApplied(UUID.randomUUID(), 500, 50));
assertEquals(500, meta.lastAppliedIndex());
}
+ @Test
+ void testLastAppliedTerm() {
+ PartitionMeta meta = new PartitionMeta();
+
+ assertEquals(0, meta.lastAppliedTerm());
+
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
Review Comment:
I think that `assertDoesNotThrow` is a bit redundant in these tests. This method is only useful when you don't have any other assertions in a test
##########
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:
Do we also need to write `lastGroupConfig` here?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/StateMachine.java:
##########
@@ -96,6 +97,16 @@ public interface StateMachine {
*/
void onConfigurationCommitted(final Configuration conf);
+ /**
+ * Invoked when a configuration has been committed to the group. This is different from
+ * {@link #onConfigurationCommitted( Configuration)} as full configuration entry is provided.
Review Comment:
```suggestion
* {@link #onConfigurationCommitted(Configuration)} as full configuration entry is provided.
```
##########
modules/raft-client/src/main/java/org/apache/ignite/raft/client/service/CommittedConfiguration.java:
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.raft.client.service;
+
+import java.util.List;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * A committed RAFT configuration with index and term at which it was committed.
+ */
+public class CommittedConfiguration {
+ private final long index;
+ private final long term;
+
+ @IgniteToStringInclude
+ private final List<String> peers;
+ @IgniteToStringInclude
+ private final List<String> learners;
+
+ @Nullable
+ @IgniteToStringInclude
+ private final List<String> oldPeers;
+ @Nullable
+ @IgniteToStringInclude
+ private final List<String> oldLearners;
+
+ /**
+ * Creates a new instance.
+ */
+ public CommittedConfiguration(long index, long term, List<String> peers, List<String> learners, @Nullable List<String> oldPeers,
+ @Nullable List<String> oldLearners) {
+ this.index = index;
+ this.term = term;
+ this.peers = List.copyOf(peers);
+ this.learners = List.copyOf(learners);
+ this.oldPeers = oldPeers == null ? null : List.copyOf(oldPeers);
+ this.oldLearners = oldLearners == null ? oldLearners : List.copyOf(oldLearners);
+ }
+
+ /**
+ * Returns RAFT index corresponding to this configuration entry. Never 0.
Review Comment:
I guess that `Never 0` should be replaced with `always positive`
##########
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;
}
+
+ // Tests use such strange metas, so we have to protect... in production, these are never null.
Review Comment:
What tests? Are they ours or from Jraft?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/GroupConfiguration.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.storage;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * A RAFT group configuration at which it was committed.
+ */
+public class GroupConfiguration implements Serializable {
Review Comment:
Shall we rename it to `RaftGroupConfiguration`?
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/raft/snapshot/outgoing/OutgoingSnapshotMvDataStreamingTest.java:
##########
@@ -281,6 +280,43 @@ void sendsOutOfOrderRowsWithHighestPriority() {
assertThat(response.rows().get(1).rowId(), is(rowId1.uuid()));
}
+ @Test
+ void doesNotEnqueueMissingRows() {
+ when(mvPartitionStorage.scanVersions(rowIdOutOfOrder)).thenReturn(Cursor.fromIterator(emptyIterator()));
Review Comment:
There is a `CursorUtils#emptyCursor` method
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/storage/state/rocksdb/TxStateRocksDbStorage.java:
##########
@@ -115,11 +118,13 @@ public TxStateRocksDbStorage(
this.persistedTierReadOptions = persistedTierReadOptions;
this.partitionId = partitionId;
this.tableStorage = tableStorage;
- this.lastAppliedIndexKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
+ this.lastAppliedIndexAndTermKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
.putShort((short) partitionId)
.array();
- lastAppliedIndex = readLastAppliedIndex(readOptions);
+ byte[] indexAndTermBytes = readLastAppliedIndexAndTerm(readOptions);
Review Comment:
That's actually interesting: why do you store index and term together here, but not in the Partition Storage?
##########
modules/raft-client/src/main/java/org/apache/ignite/raft/client/service/CommittedConfiguration.java:
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.raft.client.service;
+
+import java.util.List;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * A committed RAFT configuration with index and term at which it was committed.
+ */
+public class CommittedConfiguration {
+ private final long index;
+ private final long term;
+
+ @IgniteToStringInclude
+ private final List<String> peers;
+ @IgniteToStringInclude
+ private final List<String> learners;
+
+ @Nullable
+ @IgniteToStringInclude
+ private final List<String> oldPeers;
+ @Nullable
+ @IgniteToStringInclude
+ private final List<String> oldLearners;
+
+ /**
+ * Creates a new instance.
+ */
+ public CommittedConfiguration(long index, long term, List<String> peers, List<String> learners, @Nullable List<String> oldPeers,
+ @Nullable List<String> oldLearners) {
+ this.index = index;
+ this.term = term;
+ this.peers = List.copyOf(peers);
+ this.learners = List.copyOf(learners);
+ this.oldPeers = oldPeers == null ? null : List.copyOf(oldPeers);
+ this.oldLearners = oldLearners == null ? oldLearners : List.copyOf(oldLearners);
Review Comment:
```suggestion
this.oldLearners = oldLearners == null ? null : List.copyOf(oldLearners);
```
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -588,6 +599,26 @@ public void result(Serializable res) {
}
}
+ @Override
+ public void onRawConfigurationCommitted(ConfigurationEntry entry) {
+ boolean hasOldConf = entry.getOldConf() != null && entry.getOldConf().getPeers() != null;
+
+ CommittedConfiguration committedConf = new CommittedConfiguration(
+ entry.getId().getIndex(),
+ entry.getId().getTerm(),
+ peersIdsToStrings(entry.getConf().getPeers()),
+ peersIdsToStrings(entry.getConf().getLearners()),
+ hasOldConf ? peersIdsToStrings(entry.getOldConf().getPeers()) : null,
+ hasOldConf ? peersIdsToStrings(entry.getOldConf().getLearners()) : null
+ );
+
+ listener.onConfigurationCommitted(committedConf);
+ }
+
+ private static List<String> peersIdsToStrings(Collection<PeerId> peerIds) {
+ return peerIds.stream().map(PeerId::toString).collect(toList());
Review Comment:
I would suggest using `toUnmodifiableList` here, because you will copy this list inside `ComittedConfiguration` otherwise
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/FSMCallerImpl.java:
##########
@@ -497,6 +497,17 @@ private void doCommitted(final long committedIndex) {
final LogEntry logEntry = iterImpl.entry();
if (logEntry.getType() != EnumOutter.EntryType.ENTRY_TYPE_DATA) {
if (logEntry.getType() == EnumOutter.EntryType.ENTRY_TYPE_CONFIGURATION) {
+ ConfigurationEntry configurationEntry = new ConfigurationEntry(
+ logEntry.getId().copy(),
+ new Configuration(logEntry.getPeers(), logEntry.getLearners()),
+ null
+ );
+ if (logEntry.getOldPeers() != null && !logEntry.getOldPeers().isEmpty()) {
+ configurationEntry.setOldConf(new Configuration(logEntry.getOldPeers(), logEntry.getOldLearners()));
+ }
+
+ this.fsm.onRawConfigurationCommitted(configurationEntry);
+
if (logEntry.getOldPeers() != null && !logEntry.getOldPeers().isEmpty()) {
// Joint stage is not supposed to be noticeable by end users.
this.fsm.onConfigurationCommitted(new Configuration(iterImpl.entry().getPeers()));
Review Comment:
Not related to this PR, but why do we only provide peers configuration here and not learners configuration?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/FSMCallerImpl.java:
##########
@@ -497,6 +497,17 @@ private void doCommitted(final long committedIndex) {
final LogEntry logEntry = iterImpl.entry();
if (logEntry.getType() != EnumOutter.EntryType.ENTRY_TYPE_DATA) {
if (logEntry.getType() == EnumOutter.EntryType.ENTRY_TYPE_CONFIGURATION) {
+ ConfigurationEntry configurationEntry = new ConfigurationEntry(
+ logEntry.getId().copy(),
+ new Configuration(logEntry.getPeers(), logEntry.getLearners()),
+ null
Review Comment:
Looks like `ConfigurationEntry` does not expect to be provided with `null` here, you should use an empty configuration instead
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java:
##########
@@ -71,20 +71,39 @@ public interface MvPartitionStorage extends ManuallyCloseable {
CompletableFuture<Void> flush();
/**
- * Index of the highest write command applied to the storage. {@code 0} if index is unknown.
+ * Index of the highest write command applied to the storage. {@code 0} if the index is unknown.
*/
long lastAppliedIndex();
/**
- * Sets the last applied index value.
+ * Term of the highest write command applied to the storage. {@code 0} if the term is unknown.
Review Comment:
What do you mean by `highest write command`?
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/raft/snapshot/outgoing/OutgoingSnapshotsManagerTest.java:
##########
@@ -59,15 +67,22 @@ void emptyOngoingSnapshotsIfNoSnapshotWasRegistered() {
}
@Test
- void registersSnapshot() {
- OutgoingSnapshot snapshot = mock(OutgoingSnapshot.class);
- doReturn(partitionKey).when(snapshot).partitionKey();
+ void startsSnapshot() {
+ MvPartitionStorage mvPartitionStorage = mock(MvPartitionStorage.class);
+
+ when(partitionAccess.partitionKey()).thenReturn(partitionKey);
+ when(partitionAccess.mvPartitionStorage()).thenReturn(mvPartitionStorage);
+ when(partitionAccess.txStatePartitionStorage()).thenReturn(mock(TxStateStorage.class));
+
+ when(mvPartitionStorage.committedGroupConfiguration()).thenReturn(mock(GroupConfiguration.class));
+
+ OutgoingSnapshot snapshot = new OutgoingSnapshot(UUID.randomUUID(), partitionAccess);
manager.startOutgoingSnapshot(UUID.randomUUID(), snapshot);
Review Comment:
Do you test that `startOutgoingSnapshot` does not throw any exceptions?
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageTest.java:
##########
@@ -1323,6 +1325,35 @@ void committedMethodCallDoesNotInterfereWithIteratingOverScanCursor(ScanTimestam
}
}
+ @Test
+ void groupConfigurationOnEmptyStorageIsNull() {
+ assertThat(storage.committedGroupConfiguration(), is(nullValue()));
+ }
+
+ @Test
+ void groupConfigurationIsUpdated() {
+ GroupConfiguration configToSave = new GroupConfiguration(
+ List.of("peer1", "peer2"),
+ List.of("learner1", "learner2"),
+ List.of("old-peer1", "old-peer2"),
+ List.of("old-learner1", "old-learner2")
+ );
+
+ storage.runConsistently(() -> {
+ storage.committedGroupConfiguration(configToSave);
+
+ return null;
+ });
+
+ GroupConfiguration returnedConfig = storage.committedGroupConfiguration();
+
+ assertThat(returnedConfig, is(notNullValue()));
+ assertThat(returnedConfig.peers(), is(List.of("peer1", "peer2")));
Review Comment:
`GroupConfiguration` has `equals` implemented, so you can compare the objects directly
##########
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> rowIdsToRows(MvPartitionStorage storage) {
Review Comment:
I think this method should be called the opposite
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021383421
##########
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:
It is an insertion happening in memory inside a batch. It seems that even if we change it to one insert it will be really difficult to see any difference, even under load (where we'll probably be capped by FS performance, not by this difference).
Anyway, I don't have a strong opinion on this. If you believe we should change this, please let me know.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021339972
##########
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:
`FSMCallerImpl:672` (in `doSnapshotLoad()`):
```
if (!this.fsm.onSnapshotLoad(reader)) {
```
Which, in turn, calls `getPath()`, the result of which is later ignored by our `PartitionListener#onSnapshotLoad()`. But if an exception is thrown, it impedes the snapshot load process.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020237606
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaTest.java:
##########
@@ -41,15 +44,45 @@ void testLastAppliedIndex() {
assertEquals(0, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(null, 100));
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
assertEquals(100, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(UUID.randomUUID(), 500));
+ assertDoesNotThrow(() -> meta.lastApplied(UUID.randomUUID(), 500, 50));
assertEquals(500, meta.lastAppliedIndex());
}
+ @Test
+ void testLastAppliedTerm() {
+ PartitionMeta meta = new PartitionMeta();
+
+ assertEquals(0, meta.lastAppliedTerm());
+
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
Review Comment:
When you just call a method, it's not immediately clear what you want to check here. After a second of thinking you understand: oh, they are making sure it does not explode. On the other hand, using `assertDoesNotThrow()` makes this expectation explicit. I would prefer to leave it 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020292251
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/storage/state/rocksdb/TxStateRocksDbStorage.java:
##########
@@ -115,11 +118,13 @@ public TxStateRocksDbStorage(
this.persistedTierReadOptions = persistedTierReadOptions;
this.partitionId = partitionId;
this.tableStorage = tableStorage;
- this.lastAppliedIndexKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
+ this.lastAppliedIndexAndTermKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
.putShort((short) partitionId)
.array();
- lastAppliedIndex = readLastAppliedIndex(readOptions);
+ byte[] indexAndTermBytes = readLastAppliedIndexAndTerm(readOptions);
Review Comment:
Also, for TX partition storage meta is simpler. We don't need to store configuration there, which is A) many bytes, and B) it is updated independently from index+term. So this 'squashing to same value' would not work well for MV partition meta, but it does work for TX data partition meta.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020265798
##########
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:
Good catch!
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021331930
##########
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:
We could, but should we? This makes a code arguably more difficult to understand.
In `TxStateRocksDbStorage` this is what I did: I store them under the same key, but there we don't use a separate meta CF, so we only have 1 key that can be used in a 'not too ugly way'.
--
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] alievmirza commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
alievmirza commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021181078
##########
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.
Review Comment:
I didn't get this comment
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020276138
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaTest.java:
##########
@@ -41,15 +44,45 @@ void testLastAppliedIndex() {
assertEquals(0, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(null, 100));
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
assertEquals(100, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(UUID.randomUUID(), 500));
+ assertDoesNotThrow(() -> meta.lastApplied(UUID.randomUUID(), 500, 50));
assertEquals(500, meta.lastAppliedIndex());
}
+ @Test
+ void testLastAppliedTerm() {
+ PartitionMeta meta = new PartitionMeta();
+
+ assertEquals(0, meta.lastAppliedTerm());
+
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
Review Comment:
Oops, I've confused the situation. Now I know the answer: I did like it's done because there is another test that was written before me about `index`. I wanted to avoid, ahem, stylistic differences.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020267205
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/raft/snapshot/outgoing/OutgoingSnapshotMvDataStreamingTest.java:
##########
@@ -281,6 +280,43 @@ void sendsOutOfOrderRowsWithHighestPriority() {
assertThat(response.rows().get(1).rowId(), is(rowId1.uuid()));
}
+ @Test
+ void doesNotEnqueueMissingRows() {
+ when(mvPartitionStorage.scanVersions(rowIdOutOfOrder)).thenReturn(Cursor.fromIterator(emptyIterator()));
Review Comment:
Switched to it. BTW, why wouldn't we have that method on the `Cursor` itself? It would increase its visibility for developers.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020242562
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/FSMCallerImpl.java:
##########
@@ -497,6 +497,17 @@ private void doCommitted(final long committedIndex) {
final LogEntry logEntry = iterImpl.entry();
if (logEntry.getType() != EnumOutter.EntryType.ENTRY_TYPE_DATA) {
if (logEntry.getType() == EnumOutter.EntryType.ENTRY_TYPE_CONFIGURATION) {
+ ConfigurationEntry configurationEntry = new ConfigurationEntry(
+ logEntry.getId().copy(),
+ new Configuration(logEntry.getPeers(), logEntry.getLearners()),
+ null
+ );
+ if (logEntry.getOldPeers() != null && !logEntry.getOldPeers().isEmpty()) {
+ configurationEntry.setOldConf(new Configuration(logEntry.getOldPeers(), logEntry.getOldLearners()));
+ }
+
+ this.fsm.onRawConfigurationCommitted(configurationEntry);
+
if (logEntry.getOldPeers() != null && !logEntry.getOldPeers().isEmpty()) {
// Joint stage is not supposed to be noticeable by end users.
this.fsm.onConfigurationCommitted(new Configuration(iterImpl.entry().getPeers()));
Review Comment:
They probably think that a state machine might have some interest in the current peers, but not learners. I have no idea why, to be honest.
--
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 #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020244668
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/FSMCallerImpl.java:
##########
@@ -497,6 +497,17 @@ private void doCommitted(final long committedIndex) {
final LogEntry logEntry = iterImpl.entry();
if (logEntry.getType() != EnumOutter.EntryType.ENTRY_TYPE_DATA) {
if (logEntry.getType() == EnumOutter.EntryType.ENTRY_TYPE_CONFIGURATION) {
+ ConfigurationEntry configurationEntry = new ConfigurationEntry(
+ logEntry.getId().copy(),
+ new Configuration(logEntry.getPeers(), logEntry.getLearners()),
+ null
+ );
+ if (logEntry.getOldPeers() != null && !logEntry.getOldPeers().isEmpty()) {
+ configurationEntry.setOldConf(new Configuration(logEntry.getOldPeers(), logEntry.getOldLearners()));
+ }
+
+ this.fsm.onRawConfigurationCommitted(configurationEntry);
+
if (logEntry.getOldPeers() != null && !logEntry.getOldPeers().isEmpty()) {
// Joint stage is not supposed to be noticeable by end users.
this.fsm.onConfigurationCommitted(new Configuration(iterImpl.entry().getPeers()));
Review Comment:
ok, let's hope that's not a bug
--
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] alievmirza commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
alievmirza commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021181078
##########
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.
Review Comment:
I didn't understand this comment
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020266482
##########
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;
}
+
+ // Tests use such strange metas, so we have to protect... in production, these are never null.
Review Comment:
JRaft tests. I've clarified the comment.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020286986
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaTest.java:
##########
@@ -41,15 +44,45 @@ void testLastAppliedIndex() {
assertEquals(0, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(null, 100));
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
assertEquals(100, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(UUID.randomUUID(), 500));
+ assertDoesNotThrow(() -> meta.lastApplied(UUID.randomUUID(), 500, 50));
assertEquals(500, meta.lastAppliedIndex());
}
+ @Test
+ void testLastAppliedTerm() {
+ PartitionMeta meta = new PartitionMeta();
+
+ assertEquals(0, meta.lastAppliedTerm());
+
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
Review Comment:
Removed it in first 3 test methods
--
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] ibessonov commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021384428
##########
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:
Assertion fits better, I agree
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021353381
##########
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:
In JRaft, the first entry in log (index=1, term=1) is always a configuration. So, if at least 1 log entry is applied, `committedGroupConfiguration()` will be non-null. Also, a leader might with to install a snapshot only if it is ahead of a follower, so it has something applied, so in this code the returned config` is never null.
I replaced the check with an assertion. WDYT?
--
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] ibessonov merged pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
ibessonov merged PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021357515
##########
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:
I clarified the comment. I made the condition the most protective just in case as here we are protecting from test code. In production, this doesn't matter, neither peers no learners can be null.
--
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 #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020244236
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaTest.java:
##########
@@ -41,15 +44,45 @@ void testLastAppliedIndex() {
assertEquals(0, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(null, 100));
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
assertEquals(100, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(UUID.randomUUID(), 500));
+ assertDoesNotThrow(() -> meta.lastApplied(UUID.randomUUID(), 500, 50));
assertEquals(500, meta.lastAppliedIndex());
}
+ @Test
+ void testLastAppliedTerm() {
+ PartitionMeta meta = new PartitionMeta();
+
+ assertEquals(0, meta.lastAppliedTerm());
+
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
Review Comment:
But this is not true: you actually check that `lastApplied` sets the index and the term, which is checked below using `assertEquals`
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021320950
##########
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:
I added a description to https://issues.apache.org/jira/browse/IGNITE-18118
This discusses a couple of alternatives and requires a discussion.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021320950
##########
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:
I added a description to https://issues.apache.org/jira/browse/IGNITE-18118
This mentions a couple of alternatives and requires a discussion.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020290598
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/storage/state/rocksdb/TxStateRocksDbStorage.java:
##########
@@ -115,11 +118,13 @@ public TxStateRocksDbStorage(
this.persistedTierReadOptions = persistedTierReadOptions;
this.partitionId = partitionId;
this.tableStorage = tableStorage;
- this.lastAppliedIndexKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
+ this.lastAppliedIndexAndTermKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
.putShort((short) partitionId)
.array();
- lastAppliedIndex = readLastAppliedIndex(readOptions);
+ byte[] indexAndTermBytes = readLastAppliedIndexAndTerm(readOptions);
Review Comment:
Different original authors, I guess
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021373516
##########
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:
Looks like we can as term must be monothonic for committed log entries (and here we only have committed ones).
Applied this change in all 3 places.
--
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] ibessonov commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021377906
##########
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:
Maybe I'm preemptively thinking about performance. One insertion is better than two
--
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] ibessonov commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021383753
##########
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:
Ok, didn't encounter this situation before. A little comment would be nice to have :)
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020267525
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/raft/snapshot/outgoing/OutgoingSnapshotsManagerTest.java:
##########
@@ -59,15 +67,22 @@ void emptyOngoingSnapshotsIfNoSnapshotWasRegistered() {
}
@Test
- void registersSnapshot() {
- OutgoingSnapshot snapshot = mock(OutgoingSnapshot.class);
- doReturn(partitionKey).when(snapshot).partitionKey();
+ void startsSnapshot() {
+ MvPartitionStorage mvPartitionStorage = mock(MvPartitionStorage.class);
+
+ when(partitionAccess.partitionKey()).thenReturn(partitionKey);
+ when(partitionAccess.mvPartitionStorage()).thenReturn(mvPartitionStorage);
+ when(partitionAccess.txStatePartitionStorage()).thenReturn(mock(TxStateStorage.class));
+
+ when(mvPartitionStorage.committedGroupConfiguration()).thenReturn(mock(GroupConfiguration.class));
+
+ OutgoingSnapshot snapshot = new OutgoingSnapshot(UUID.randomUUID(), partitionAccess);
manager.startOutgoingSnapshot(UUID.randomUUID(), snapshot);
Review Comment:
Sure, this was the intention. Thanks, added the assertion
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020273283
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/storage/state/rocksdb/TxStateRocksDbStorage.java:
##########
@@ -115,11 +118,13 @@ public TxStateRocksDbStorage(
this.persistedTierReadOptions = persistedTierReadOptions;
this.partitionId = partitionId;
this.tableStorage = tableStorage;
- this.lastAppliedIndexKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
+ this.lastAppliedIndexAndTermKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
.putShort((short) partitionId)
.array();
- lastAppliedIndex = readLastAppliedIndex(readOptions);
+ byte[] indexAndTermBytes = readLastAppliedIndexAndTerm(readOptions);
Review Comment:
In the partition storage, we store the metadata (including index and term) in a separate column family. Here, there was just one metadata originally (last applied index), so the author employed a hack: a special key was used to store the index in the same column family that hosts the data. Now we need to store 2 pieces of metadata, so it becomes a bit ugly to increase the number of keys.
I invented the following solutions:
1. Add `meta` CF as it is done for MV data storage. This is a bit cumbersome.
2. Construct metadata keys like this: `concat(partitionId, 0)` for index, `concat(partitionId, 1)` for term and then skip them when iterating over the keys in `scan()`. I implemented this first, but it causes a penalty of reading the key to analyze its length on each iteration, and it's a bit cumbersome as well.
3. Reuse the same (the only) metadata key to store index together with term. They are always updated together, so this looked like a natural solution, I chose this one.
--
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 #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020285640
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java:
##########
@@ -71,20 +71,39 @@ public interface MvPartitionStorage extends ManuallyCloseable {
CompletableFuture<Void> flush();
/**
- * Index of the highest write command applied to the storage. {@code 0} if index is unknown.
+ * Index of the highest write command applied to the storage. {@code 0} if the index is unknown.
*/
long lastAppliedIndex();
/**
- * Sets the last applied index value.
+ * Term of the highest write command applied to the storage. {@code 0} if the term is unknown.
Review Comment:
Can you state that explicitly? It's too confusing otherwise
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020261037
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java:
##########
@@ -71,20 +71,39 @@ public interface MvPartitionStorage extends ManuallyCloseable {
CompletableFuture<Void> flush();
/**
- * Index of the highest write command applied to the storage. {@code 0} if index is unknown.
+ * Index of the highest write command applied to the storage. {@code 0} if the index is unknown.
*/
long lastAppliedIndex();
/**
- * Sets the last applied index value.
+ * Term of the highest write command applied to the storage. {@code 0} if the term is unknown.
Review Comment:
It's the command that was written with the highest 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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021328655
##########
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:
Are you asking about https://github.com/apache/ignite-3/commit/aacb6ab15651a49f08a3841760537823cb8c34cf ? If yes, then yes, this commit was merged to `main` before I branched from `main`, my branch contains it.
Am I doing something wrong? I can see in my tests that peers are now identified with strings like `itrst_t_0` instead of `127.0.1.1:3344`, but everything seems working fine.
##########
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:
Are you asking about https://github.com/apache/ignite-3/commit/aacb6ab15651a49f08a3841760537823cb8c34cf ? If yes, then sure, this commit was merged to `main` before I branched from `main`, my branch contains it.
Am I doing something wrong? I can see in my tests that peers are now identified with strings like `itrst_t_0` instead of `127.0.1.1:3344`, but everything seems working fine.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021390690
##########
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.
Review Comment:
I clarified the comment a bit.
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1021283999
##########
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.
Review Comment:
Take a look at `FSMCallerTest:166` (`testOnSnapshotLoad()`). The test constructs a snapshot meta without peers/learners info (so `peers()` and `learners()` return `null`). Here, the `if` protects from such strange metas.
In production code, `peers()` and `learners()` on `SnapshotMeta` never return `null`.
--
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 #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020284567
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaTest.java:
##########
@@ -41,15 +44,45 @@ void testLastAppliedIndex() {
assertEquals(0, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(null, 100));
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
assertEquals(100, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(UUID.randomUUID(), 500));
+ assertDoesNotThrow(() -> meta.lastApplied(UUID.randomUUID(), 500, 50));
assertEquals(500, meta.lastAppliedIndex());
}
+ @Test
+ void testLastAppliedTerm() {
+ PartitionMeta meta = new PartitionMeta();
+
+ assertEquals(0, meta.lastAppliedTerm());
+
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
Review Comment:
Can we change the other tests instead? This looks quite strange to me
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaTest.java:
##########
@@ -41,15 +44,45 @@ void testLastAppliedIndex() {
assertEquals(0, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(null, 100));
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
assertEquals(100, meta.lastAppliedIndex());
- assertDoesNotThrow(() -> meta.lastAppliedIndex(UUID.randomUUID(), 500));
+ assertDoesNotThrow(() -> meta.lastApplied(UUID.randomUUID(), 500, 50));
assertEquals(500, meta.lastAppliedIndex());
}
+ @Test
+ void testLastAppliedTerm() {
+ PartitionMeta meta = new PartitionMeta();
+
+ assertEquals(0, meta.lastAppliedTerm());
+
+ assertDoesNotThrow(() -> meta.lastApplied(null, 100, 10));
Review Comment:
Can we change the other test instead? This looks quite strange to me
--
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 #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020286865
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/raft/snapshot/outgoing/OutgoingSnapshotMvDataStreamingTest.java:
##########
@@ -281,6 +280,43 @@ void sendsOutOfOrderRowsWithHighestPriority() {
assertThat(response.rows().get(1).rowId(), is(rowId1.uuid()));
}
+ @Test
+ void doesNotEnqueueMissingRows() {
+ when(mvPartitionStorage.scanVersions(rowIdOutOfOrder)).thenReturn(Cursor.fromIterator(emptyIterator()));
Review Comment:
We used to, but you will have to make `EMPTY_CURSOR` public, which I don't like, so I decided to move it here (like `Collections.emptyList()`. Maybe we can add a method to `Cursor` which delegates to `CursorUtils` but I don't know if it's a good idea
--
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 #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020287433
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/storage/state/rocksdb/TxStateRocksDbStorage.java:
##########
@@ -115,11 +118,13 @@ public TxStateRocksDbStorage(
this.persistedTierReadOptions = persistedTierReadOptions;
this.partitionId = partitionId;
this.tableStorage = tableStorage;
- this.lastAppliedIndexKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
+ this.lastAppliedIndexAndTermKey = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.BIG_ENDIAN)
.putShort((short) partitionId)
.array();
- lastAppliedIndex = readLastAppliedIndex(readOptions);
+ byte[] indexAndTermBytes = readLastAppliedIndexAndTerm(readOptions);
Review Comment:
Why don't we use the same approach in the Partition Storage?
--
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] rpuch commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1331:
URL: https://github.com/apache/ignite-3/pull/1331#discussion_r1020292722
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java:
##########
@@ -71,20 +71,39 @@ public interface MvPartitionStorage extends ManuallyCloseable {
CompletableFuture<Void> flush();
/**
- * Index of the highest write command applied to the storage. {@code 0} if index is unknown.
+ * Index of the highest write command applied to the storage. {@code 0} if the index is unknown.
*/
long lastAppliedIndex();
/**
- * Sets the last applied index value.
+ * Term of the highest write command applied to the storage. {@code 0} if the term is unknown.
Review Comment:
Done. Please take a look whether it's ok now.
--
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] ibessonov commented on a diff in pull request #1331: IGNITE-18122 Track last applied term and group config in storages
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org