sashapolo commented on code in PR #1983:
URL: https://github.com/apache/ignite-3/pull/1983#discussion_r1177390288
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -878,6 +879,48 @@ public void nodeWithDataTest() throws InterruptedException
{
checkTableWithData(ignite, TABLE_NAME);
}
+ /**
+ * Restarts the node which stores some data.
+ */
+ @Test
+ public void nodeWithDataAndIndexRebuildTest() throws InterruptedException {
+ IgniteImpl ignite = startNode(0);
+
+ createTableWithData(List.of(ignite), TABLE_NAME, 1, 20);
+
+ TableImpl table = (TableImpl) ignite.tables().table(TABLE_NAME);
+
+ InternalTableImpl internalTable = (InternalTableImpl)
table.internalTable();
+
+ for (int i = 0; i < 20; i++) {
+ // Flush data on disk, so that we will have a snapshot to read on
restart.
+ internalTable.storage().getMvPartition(i).flush().join();
+ }
+
+ // Add more data, so that on restart there will be a index rebuilding
operation.
+ try (Session session = ignite.sql().createSession()) {
+ for (int i = 0; i < 100; i++) {
+ session.execute(null, "INSERT INTO " + TABLE_NAME + "(id,
name) VALUES (?, ?)",
+ i + 500, VALUE_PRODUCER.apply(i + 500));
Review Comment:
What's `+ 500` for?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -878,6 +879,48 @@ public void nodeWithDataTest() throws InterruptedException
{
checkTableWithData(ignite, TABLE_NAME);
}
+ /**
+ * Restarts the node which stores some data.
+ */
+ @Test
+ public void nodeWithDataAndIndexRebuildTest() throws InterruptedException {
+ IgniteImpl ignite = startNode(0);
+
+ createTableWithData(List.of(ignite), TABLE_NAME, 1, 20);
+
+ TableImpl table = (TableImpl) ignite.tables().table(TABLE_NAME);
+
+ InternalTableImpl internalTable = (InternalTableImpl)
table.internalTable();
+
+ for (int i = 0; i < 20; i++) {
+ // Flush data on disk, so that we will have a snapshot to read on
restart.
+ internalTable.storage().getMvPartition(i).flush().join();
Review Comment:
Let's not use `join` here and use an assertion instead
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -878,6 +879,48 @@ public void nodeWithDataTest() throws InterruptedException
{
checkTableWithData(ignite, TABLE_NAME);
}
+ /**
+ * Restarts the node which stores some data.
+ */
+ @Test
+ public void nodeWithDataAndIndexRebuildTest() throws InterruptedException {
+ IgniteImpl ignite = startNode(0);
+
+ createTableWithData(List.of(ignite), TABLE_NAME, 1, 20);
+
+ TableImpl table = (TableImpl) ignite.tables().table(TABLE_NAME);
+
+ InternalTableImpl internalTable = (InternalTableImpl)
table.internalTable();
+
+ for (int i = 0; i < 20; i++) {
+ // Flush data on disk, so that we will have a snapshot to read on
restart.
+ internalTable.storage().getMvPartition(i).flush().join();
Review Comment:
Also, these flushes can be done in parallel, no need to wait for them
sequentially
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/snapshot/SnapshotExecutorImpl.java:
##########
@@ -250,7 +250,13 @@ public boolean init(final SnapshotExecutorOptions opts) {
final FirstSnapshotLoadDone done = new FirstSnapshotLoadDone(reader);
Requires.requireTrue(this.fsmCaller.onSnapshotLoad(done));
try {
- done.waitForRun();
+ // TODO: IGNITE-19363 We want to avoid deadlock for now, but this
is an ad-hoc decision.
+ // We don't wait for the partition's snapshot load closure to
finish here.
+ if (!node.getNodeId().getGroupId().contains("part")) {
+ done.waitForRun();
+ } else {
+ done.status = new Status();
Review Comment:
Also, there's a `Status.OK()` method, which looks a little bit more
descriptive
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/snapshot/SnapshotExecutorImpl.java:
##########
@@ -250,7 +250,13 @@ public boolean init(final SnapshotExecutorOptions opts) {
final FirstSnapshotLoadDone done = new FirstSnapshotLoadDone(reader);
Requires.requireTrue(this.fsmCaller.onSnapshotLoad(done));
try {
- done.waitForRun();
+ // TODO: IGNITE-19363 We want to avoid deadlock for now, but this
is an ad-hoc decision.
+ // We don't wait for the partition's snapshot load closure to
finish here.
+ if (!node.getNodeId().getGroupId().contains("part")) {
+ done.waitForRun();
+ } else {
+ done.status = new Status();
Review Comment:
How does this work? Don't we have a race here with setting the task status?
What will happen if snapshot loading fails?
--
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]
