github-actions[bot] commented on code in PR #4442:
URL: https://github.com/apache/iggy/pull/4442#discussion_r4272766724
##########
core/integration/tests/cluster/crash_recovery_corruption.rs:
##########
@@ -568,6 +610,113 @@ async fn
given_a_torn_index_tail_when_a_node_recovers_should_not_misalign_subseq
});
}
+/// Primary-side counterpart of the backup torn-tail specs: the torn file is
+/// the roster leader's, asserted to be the partition primary too. A restarted
+/// node rejoins as a backup under an elected successor, so the post-torn
+/// batches reach its reopened segment by replication. Same contract as the
+/// backup specs: boot over the tear, survive the NEXT restart after that
+/// append, every acked offset reads back, at-rest `.log` bytes identical.
+async fn require_torn_primary_recovery(harness: &mut TestHarness, extension:
&str, garbage: usize) {
+ let client = harness.tcp_root_client().await.unwrap();
+ create_stream_and_topic(&client, Durability::Persisted).await;
+ let mut acked = produce_acked(&client, "pre-torn", 30).await;
+ let pre_payloads: Vec<String> = acked.iter().map(|(_, payload)|
payload.clone()).collect();
+ for node in 0..harness.cluster_size() {
+ wait_until_node_holds_payloads(
+ harness,
+ node,
+ &pre_payloads,
+ FLUSH_INSTALL_TIMEOUT,
+ "eager flush before the surgery",
+ )
+ .await;
+ }
+ drop(client);
+
+ let primary = disk::leader_node_index(harness).await;
+ let partition_view =
disk::read_partition_superblock_state(&harness.node(primary).data_path())
+ .map_or(INITIAL_VIEW, |state| state.view);
+ assert_eq!(
+ partition_view as usize % harness.cluster_size(),
+ primary,
+ "the surgery victim must be the partition primary as well as the
roster leader, or \
+ the spec tears a backup's segment under a primary-side name"
+ );
+ harness.stop_node(primary).expect("stop the primary");
+ let torn_file =
find_active_segment_file(&harness.node(primary).data_path(), extension);
+ append_garbage(&torn_file, garbage);
+
+ harness.restart_node(primary).unwrap_or_else(|error| {
+ panic!("a primary must boot over a torn segment .{extension} tail:
{error}")
+ });
+
+ let successor = wait_for_successor_leader(harness, primary,
CONVERGE_TIMEOUT).await;
+ let client = harness
+ .root_client_for_node(successor)
+ .await
+ .expect("connect a producer to the successor primary");
+ acked.extend(produce_acked(&client, "post-torn", 30).await);
+ let all_payloads: Vec<String> = acked.iter().map(|(_, payload)|
payload.clone()).collect();
+ for node in 0..harness.cluster_size() {
+ wait_until_node_holds_payloads(
+ harness,
+ node,
+ &all_payloads,
+ CONVERGE_TIMEOUT,
+ "replication onto the reopened segment after the torn-tail
recovery",
+ )
+ .await;
+ }
+
+ harness.restart_node(primary).unwrap_or_else(|error| {
+ panic!(
+ "a node must restart cleanly over a segment it repaired as the
primary and then \
+ appended to as a backup; boot error: {error}"
+ )
+ });
+ let nodes: Vec<usize> = (0..harness.cluster_size()).collect();
+ let client = wait_until_cluster_serves(harness, &nodes,
CONVERGE_TIMEOUT).await;
+ wait_for_acked_readable(&client, &acked, CONVERGE_TIMEOUT)
+ .await
+ .unwrap_or_else(|state| {
+ panic!(
+ "every acked offset must poll back after the primary-side
torn-tail recovery: \
+ {state}"
+ )
+ });
+
+ let data_paths: Vec<PathBuf> = harness
+ .all_servers()
+ .iter()
+ .map(|server| server.data_path())
+ .collect();
+ harness
+ .stop()
+ .await
+ .expect("stop the cluster for the at-rest comparison");
+ assert_segment_logs_identical(
Review Comment:
simplification: The call reuses the local `assert_segment_logs_identical`,
which walks node 0's keys only, so a segment that a backup alone holds escapes
the comparison. Call `disk::assert_replica_data_identical(&data_paths, false)`,
which this file already uses at line 919.
##########
core/integration/tests/cluster/crash_recovery_corruption.rs:
##########
@@ -279,6 +281,49 @@ async fn pick_backup(harness: &TestHarness) -> (usize,
usize) {
(leader, backup)
}
+/// The roster leader read through node `via`, or `None` while no node is
+/// marked leader or the connection fails. The non-panicking form of
+/// `disk::leader_node_index_via`, so a poll can ride out an election.
+async fn roster_leader_via(harness: &TestHarness, via: usize) -> Option<usize>
{
Review Comment:
simplification: `roster_leader_via` repeats the roster read and port match
of `disk::leader_node_index_via` at `core/integration/src/harness/disk.rs:532`,
only to return `Option` instead of panicking. Add an `Option` form beside it
and delete this copy.
##########
core/integration/tests/cluster/crash_recovery_corruption.rs:
##########
@@ -568,6 +610,113 @@ async fn
given_a_torn_index_tail_when_a_node_recovers_should_not_misalign_subseq
});
}
+/// Primary-side counterpart of the backup torn-tail specs: the torn file is
+/// the roster leader's, asserted to be the partition primary too. A restarted
+/// node rejoins as a backup under an elected successor, so the post-torn
+/// batches reach its reopened segment by replication. Same contract as the
+/// backup specs: boot over the tear, survive the NEXT restart after that
+/// append, every acked offset reads back, at-rest `.log` bytes identical.
+async fn require_torn_primary_recovery(harness: &mut TestHarness, extension:
&str, garbage: usize) {
+ let client = harness.tcp_root_client().await.unwrap();
Review Comment:
simplification: The prologue copies lines 468-481 and 554-567 statement for
statement, and two further specs carry the same shape. Extract one
seed-and-flush helper and call it wherever this block appears.
##########
core/integration/tests/cluster/crash_recovery_corruption.rs:
##########
@@ -568,6 +610,113 @@ async fn
given_a_torn_index_tail_when_a_node_recovers_should_not_misalign_subseq
});
}
+/// Primary-side counterpart of the backup torn-tail specs: the torn file is
+/// the roster leader's, asserted to be the partition primary too. A restarted
+/// node rejoins as a backup under an elected successor, so the post-torn
+/// batches reach its reopened segment by replication. Same contract as the
+/// backup specs: boot over the tear, survive the NEXT restart after that
Review Comment:
nit: The doc claims the backup specs share the at-rest `.log` byte-identity
contract, but the backup `.index` spec at line 551 stops at the readback check.
Name the backup `.log` spec instead.
##########
core/integration/tests/cluster/crash_recovery_corruption.rs:
##########
@@ -568,6 +610,113 @@ async fn
given_a_torn_index_tail_when_a_node_recovers_should_not_misalign_subseq
});
}
+/// Primary-side counterpart of the backup torn-tail specs: the torn file is
+/// the roster leader's, asserted to be the partition primary too. A restarted
+/// node rejoins as a backup under an elected successor, so the post-torn
+/// batches reach its reopened segment by replication. Same contract as the
+/// backup specs: boot over the tear, survive the NEXT restart after that
+/// append, every acked offset reads back, at-rest `.log` bytes identical.
+async fn require_torn_primary_recovery(harness: &mut TestHarness, extension:
&str, garbage: usize) {
+ let client = harness.tcp_root_client().await.unwrap();
+ create_stream_and_topic(&client, Durability::Persisted).await;
+ let mut acked = produce_acked(&client, "pre-torn", 30).await;
+ let pre_payloads: Vec<String> = acked.iter().map(|(_, payload)|
payload.clone()).collect();
+ for node in 0..harness.cluster_size() {
+ wait_until_node_holds_payloads(
+ harness,
+ node,
+ &pre_payloads,
+ FLUSH_INSTALL_TIMEOUT,
+ "eager flush before the surgery",
+ )
+ .await;
+ }
+ drop(client);
+
+ let primary = disk::leader_node_index(harness).await;
+ let partition_view =
disk::read_partition_superblock_state(&harness.node(primary).data_path())
+ .map_or(INITIAL_VIEW, |state| state.view);
+ assert_eq!(
Review Comment:
warning: The guard asserts that the roster leader equals `partition_view %
cluster_size`, but the metadata and partition planes advance their views
independently, so a metadata-only election fails it. Pick the victim from the
partition plane, and treat the roster check as a soft precondition.
##########
core/integration/tests/cluster/crash_recovery_corruption.rs:
##########
@@ -61,6 +61,8 @@ const GARBAGE_BYTE: u8 = 0xA5;
/// Metadata ops produced before the WAL surgery, so a flip at one quarter of
/// the file provably lands ahead of many complete committed entries.
const WAL_FODDER_STREAMS: usize = 20;
+/// View of a partition group that never persisted a superblock record.
Review Comment:
nit: The comment claims a group with no persisted record has view 0, but a
group seeded at a non-zero creation view also has no record. Reword it to 'a
partition group that never left view 0', the wording at
`core/integration/src/harness/disk.rs:458`.
--
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]
