-
Notifications
You must be signed in to change notification settings - Fork 181
Add new subcommands for forest-cli snasphot
#6128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 50 commits
c0a9065
ece8b82
92c7841
e5a6b74
8175449
5be4c76
56a9c8d
64a3c48
7accd2f
2837ea3
c452a6e
c43ae0a
d0c4160
86c7631
f683b3f
365ae1f
1acb32a
c837c8a
7760b22
36847bd
e7e2148
36faa47
f0755f9
b3fcffd
c9dab4a
741918f
76c390a
ee3bb9c
820bcd5
19147cd
a317d85
04e0a25
8dc3cb6
4cfe6f3
302020b
37b8964
e0d3505
969831e
d8c06b0
119823f
2c9c0a7
c8a3e1c
d68dcc6
2d9f59a
d0f4c63
2c16735
8eb4a3c
781569e
747d7b3
9504ea1
e32f14f
5093d98
cc3d73c
07221b1
714820b
fb421be
6691f19
ac23ef0
b1667a7
d2a2f69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ use crate::cli_shared::snapshot::{self, TrustedVendor}; | |
| use crate::db::car::forest::new_forest_car_temp_path_in; | ||
| use crate::networks::calibnet; | ||
| use crate::rpc::chain::ForestChainExportDiffParams; | ||
| use crate::rpc::types::ApiExportResult; | ||
| use crate::rpc::{self, chain::ForestChainExportParams, prelude::*}; | ||
| use crate::shim::policy::policy_constants::CHAIN_FINALITY; | ||
| use anyhow::Context as _; | ||
|
|
@@ -19,6 +20,12 @@ use std::{ | |
| }; | ||
| use tokio::io::AsyncWriteExt; | ||
|
|
||
| #[derive(Debug, Clone, clap::ValueEnum)] | ||
| pub enum Format { | ||
| Json, | ||
| Text, | ||
| } | ||
|
|
||
| #[derive(Debug, Subcommand)] | ||
| pub enum SnapshotCommands { | ||
| /// Export a snapshot of the chain to `<output_path>` | ||
|
|
@@ -42,6 +49,17 @@ pub enum SnapshotCommands { | |
| #[arg(long, value_enum, default_value_t = FilecoinSnapshotVersion::V2)] | ||
| format: FilecoinSnapshotVersion, | ||
| }, | ||
| /// Show status of the current export. | ||
| ExportStatus { | ||
| /// Wait until it completes and print progress. | ||
| #[arg(long)] | ||
| wait: bool, | ||
elmattic marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced that another subcommand is better here. We already have a subcommand in forest that accepts a wait param: |
||
| /// Format of the output. `json` or `text`. | ||
| #[arg(long, value_enum, default_value_t = Format::Text)] | ||
| format: Format, | ||
| }, | ||
| /// Cancel the current export. | ||
| ExportCancel {}, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about covering these 2 new subcommands in |
||
| /// Export a diff snapshot between `from` and `to` epochs to `<output_path>` | ||
| ExportDiff { | ||
| /// `./forest_snapshot_diff_{chain}_{from}_{to}+{depth}.car.zst`. | ||
|
|
@@ -152,7 +170,7 @@ impl SnapshotCommands { | |
|
|
||
| // Manually construct RpcRequest because snapshot export could | ||
| // take a few hours on mainnet | ||
| let hash_result = client | ||
| let export_result = client | ||
| .call(ForestChainExport::request((params,))?.with_timeout(Duration::MAX)) | ||
| .await?; | ||
elmattic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
@@ -161,13 +179,106 @@ impl SnapshotCommands { | |
| _ = handle.await; | ||
|
|
||
| if !dry_run { | ||
| if let Some(hash) = hash_result { | ||
| save_checksum(&output_path, hash).await?; | ||
| match export_result.clone() { | ||
| ApiExportResult::Done(hash_opt) => { | ||
| // Move the file first; prevents orphaned checksum on persist error. | ||
| temp_path.persist(&output_path)?; | ||
| if let Some(hash) = hash_opt { | ||
| save_checksum(&output_path, hash).await?; | ||
| } | ||
| } | ||
| ApiExportResult::Cancelled => { /* no file to persist on cancel */ } | ||
| } | ||
| temp_path.persist(output_path)?; | ||
| } | ||
|
|
||
| println!("Export completed."); | ||
| match export_result { | ||
| ApiExportResult::Done(_) => { | ||
| println!("Export completed."); | ||
| } | ||
| ApiExportResult::Cancelled => { | ||
| println!("Export cancelled."); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
| Self::ExportStatus { wait, format } => { | ||
| let result = client | ||
| .call( | ||
| ForestChainExportStatus::request(())?.with_timeout(Duration::from_secs(30)), | ||
| ) | ||
| .await?; | ||
| if !result.exporting | ||
| && let Format::Text = format | ||
| { | ||
| if result.cancelled { | ||
| println!("No export in progress (last export was cancelled)"); | ||
| } else { | ||
| println!("No export in progress"); | ||
| } | ||
| return Ok(()); | ||
| } | ||
| if wait { | ||
| let elapsed = chrono::Utc::now() | ||
| .signed_duration_since(result.start_time) | ||
| .to_std() | ||
| .unwrap_or(Duration::ZERO); | ||
| let pb = ProgressBar::new(10000) | ||
| .with_elapsed(elapsed) | ||
| .with_message("Exporting"); | ||
| pb.set_style( | ||
| ProgressStyle::with_template( | ||
| "[{elapsed_precise}] [{wide_bar}] {percent}% {msg} ", | ||
| ) | ||
| .expect("indicatif template must be valid") | ||
| .progress_chars("#>-"), | ||
| ); | ||
| loop { | ||
| let result = client | ||
| .call( | ||
| ForestChainExportStatus::request(())? | ||
| .with_timeout(Duration::from_secs(30)), | ||
| ) | ||
| .await?; | ||
| if result.cancelled { | ||
| pb.set_message("Export cancelled"); | ||
| pb.abandon(); | ||
|
|
||
| return Ok(()); | ||
| } | ||
| let position = (result.progress.clamp(0.0, 1.0) * 10000.0).trunc() as u64; | ||
| pb.set_position(position); | ||
|
|
||
| if position >= 10000 { | ||
| break; | ||
| } | ||
| tokio::time::sleep(Duration::from_millis(500)).await; | ||
| } | ||
| pb.finish_with_message("Export completed"); | ||
|
|
||
| return Ok(()); | ||
| } | ||
| match format { | ||
| Format::Text => { | ||
| println!("Exporting: {:.1}%", result.progress.clamp(0.0, 1.0) * 100.0); | ||
| } | ||
| Format::Json => { | ||
| println!("{}", serde_json::to_string_pretty(&result)?); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| Self::ExportCancel {} => { | ||
| let result = client | ||
| .call( | ||
| ForestChainExportCancel::request(())?.with_timeout(Duration::from_secs(30)), | ||
| ) | ||
| .await?; | ||
| if result { | ||
| println!("Export cancelled."); | ||
| } else { | ||
| println!("No export in progress to cancel."); | ||
| } | ||
| Ok(()) | ||
| } | ||
| Self::ExportDiff { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,15 +8,58 @@ use crate::shim::clock::ChainEpoch; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::utils::db::car_stream::CarBlock; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::utils::encoding::extract_cids; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::utils::multihash::prelude::*; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use chrono::{DateTime, Utc}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use cid::Cid; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use futures::Stream; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use fvm_ipld_blockstore::Blockstore; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use parking_lot::Mutex; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use pin_project_lite::pin_project; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::borrow::Borrow; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::collections::VecDeque; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::pin::Pin; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::sync::LazyLock; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::task::{Context, Poll}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[derive(Default)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub struct ExportStatus { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub epoch: i64, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub initial_epoch: i64, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub exporting: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub cancelled: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub start_time: DateTime<Utc>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+23
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix compilation error: The Apply this diff to manually implement -#[derive(Default)]
pub struct ExportStatus {
pub epoch: i64,
pub initial_epoch: i64,
pub exporting: bool,
pub cancelled: bool,
pub start_time: DateTime<Utc>,
}
+
+impl Default for ExportStatus {
+ fn default() -> Self {
+ Self {
+ epoch: 0,
+ initial_epoch: 0,
+ exporting: false,
+ cancelled: false,
+ start_time: DateTime::<Utc>::from_timestamp(0, 0).expect("valid epoch"),
+ }
+ }
+}📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whenever you're unsure, please compile the codebase to confirm whether there's actually a compilation error. Can you do that? Start now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 238 🏁 Script executed: Length of output: 248 🌐 Web query: 💡 Result: Short answer: chrono does not provide a built-in impl Default for DateTime. You can either add your own impl or use a helper/newtype crate. Details and options:
Sources: @elmattic, I apologize - I attempted to compile the codebase but the sandbox environment encountered issues running The compilation error is real. According to the chrono crate documentation, The chrono library provides
My original suggestion stands and will fix the compilation error. You're right to ask for verification first - I should have been more certain before flagging it as critical. The issue is confirmed by the chrono documentation, even though I couldn't run cargo in this environment. Would you like me to update the suggestion with the 🧬 Code Graph Analysis Results[src/utils/encoding/cid_de_cbor.rs] (lines 16-19) [src/rpc/methods/chain.rs] (line 729-729) [src/db/parity_db.rs] (line 215-215) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your point 2. is already implemented in the https://github.com/chronotope/chrono/blob/main/src/datetime/mod.rs#L920-L924 That's why the code compiles. There is no compilation error. Ok? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elmattic, you're absolutely right, and I apologize for the confusion. I was wrong about the compilation error. Looking at the chrono documentation, Thank you for asking me to verify first and for providing the source link. I should not have flagged this as a critical issue without being certain. The code is correct as written. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please compile the codebase whenever in doubt to confirm whether there is an actual compilation error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@elmattic, understood. I will always run 🏁 Script executed: Length of output: 183 @elmattic, I acknowledge your instruction and will always attempt to compile the codebase when uncertain about compilation errors before flagging them. Unfortunately, the sandbox environment I'm currently running in has limitations (specifically, no
I apologize again for the false positive on the 🧬 Code Graph Analysis Results### File: `src/utils/encoding/cid_de_cbor.rs` — lines 16-19 ```rust pub fn extract_cids(cbor_blob: &[u8]) -> anyhow::Result { let CidVec(v) = from_slice_with_fallback(cbor_blob)?; Ok(v) } ``` ### File: `src/db/car/plain.rs` — lines 126-173 ```rust pub fn new(reader: ReaderT) -> io::Result { let mut cursor = positioned_io::Cursor::new(&reader); let position = cursor.position(); let header_v2 = read_v2_header(&mut cursor).await?; let (limit_position, version) = if let Some(header_v2) = &header_v2 { cursor.set_position(position.saturating_add(header_v2.data_offset as u64)); ( Some( cursor .stream_position()? .saturating_add(header_v2.data_size as u64), ), 2, ) } else { cursor.set_position(position); (None, 1) };✏️ Learnings added
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excuses accepted, but the shame is permanent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elmattic 😔 Understood. I'll wear it well and do better next time! 🧠 Learnings used |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub static CHAIN_EXPORT_STATUS: LazyLock<Mutex<ExportStatus>> = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| LazyLock::new(|| ExportStatus::default().into()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn update_epoch(new_value: i64) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut mutex = CHAIN_EXPORT_STATUS.lock(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.epoch = new_value; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if mutex.initial_epoch == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.initial_epoch = new_value; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn start_export() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut mutex = CHAIN_EXPORT_STATUS.lock(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.epoch = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.initial_epoch = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.exporting = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.cancelled = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.start_time = Utc::now(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
elmattic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn end_export() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut mutex = CHAIN_EXPORT_STATUS.lock(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.exporting = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn cancel_export() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut mutex = CHAIN_EXPORT_STATUS.lock(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.exporting = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex.cancelled = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn should_save_block_to_snapshot(cid: Cid) -> bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Don't include identity CIDs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // We only include raw and dagcbor, for now. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -112,6 +155,7 @@ pin_project! { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| seen: CidHashSet, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stateroot_limit_exclusive: ChainEpoch, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fail_on_dead_links: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| track_progress: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -126,6 +170,11 @@ impl<DB, T> ChainStream<DB, T> { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn track_progress(mut self, track_progress: bool) -> Self { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.track_progress = track_progress; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[allow(dead_code)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn into_seen(self) -> CidHashSet { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.seen | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -155,6 +204,7 @@ pub fn stream_chain<DB: Blockstore, T: Borrow<Tipset>, ITER: Iterator<Item = T> | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| seen: CidHashSet::default(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stateroot_limit_exclusive, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fail_on_dead_links: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| track_progress: false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -197,6 +247,9 @@ impl<DB: Blockstore, T: Borrow<Tipset>, ITER: Iterator<Item = T> + Unpin> Stream | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Iterate(epoch, block_cid, _type, cid_vec) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if *this.track_progress { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| update_epoch(*epoch); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while let Some(cid) = cid_vec.pop_front() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The link traversal implementation assumes there are three types of encoding: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 1. DAG_CBOR: needs to be reachable, so we add it to the queue and load. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -242,6 +295,9 @@ impl<DB: Blockstore, T: Borrow<Tipset>, ITER: Iterator<Item = T> + Unpin> Stream | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for block in tipset.borrow().block_headers() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let (cid, data) = block.car_block()?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if this.seen.insert(cid) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if *this.track_progress { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| update_epoch(block.epoch); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Make sure we always yield a block otherwise. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.dfs.push_back(Emit(cid, Some(data))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Hard-coded 1-second sleep creates race condition risk.
The test assumes the export will still be in-progress after
sleep 1, but this timing is not guaranteed. If the export completes faster, the test fails; if slower, you're only testing intermediate progress by coincidence. Consider:--waitflag (mentioned in PR objectives) if the export command supports itDoes the
snapshot exportcommand support a--waitflag or similar mechanism to wait for completion? If so, consider using it instead of a hard-coded sleep.I'll verify this review comment by examining the script and checking the context around the flagged issues.
🏁 Script executed:
Length of output: 2571
🏁 Script executed:
Length of output: 158
🏁 Script executed:
Length of output: 12130
Use consistent
--formatvariable throughout snapshot export lifecycle tests.The script establishes a
formatvariable (line 8) and uses it for export calls (lines 18, 21), but the status checks hard-code--format json(lines 46, 57, 66). Additionally, line 54'ssnapshot exportcall omits the format flag entirely. For consistency and to respect the test's parameterization, use--format "$format"in all three status calls and add it to line 54's export invocation:Note: The
snapshot exportcommand does not currently support a--waitflag; the 1-second sleep is the intended pattern to allow the operation to begin before checking its status.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elmattic Thank you! Let me know if you need any assistance with the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not really relevant since the export task will be cancelled and no file will be written anyway. Let’s keep the code simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elmattic Understood! Since the export will be cancelled before completion in that test section, the format parameter isn't material there. Thank you for the clarification.