[ignore] experiment and investigation #15617
Conversation
⏱️ 37m total CI duration on this PR
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| fn commit_batch(&self, batch: SchemaBatch) -> Result<()> { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The commit_batch() method currently returns success without performing any operations, which will silently discard batches instead of persisting them. This empty implementation needs to be replaced with logic that actually commits the SchemaBatch to storage to prevent data loss. Consider adding error handling and verification of the commit as well.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| let commit_thread = thread::spawn(move || { | ||
| while let Ok(batch) = self.batch_receiver.recv() { | ||
| self.commit_batch(batch).unwrap(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The spawned commit_thread needs to be joined to ensure all commits are processed before the thread terminates. Consider storing the JoinHandle and joining it at the end of run() or in Drop implementation. This prevents potential data loss from premature thread termination.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| @@ -93,8 +125,7 @@ where | |||
| let version = output.expect_last_version(); | |||
| let commit_start = Instant::now(); | |||
| let ledger_info_with_sigs = gen_li_with_sigs(block_id, root_hash, version); | |||
| self.executor.pre_commit_block(block_id).unwrap(); | |||
| self.executor.commit_ledger(ledger_info_with_sigs).unwrap(); | |||
| self.prepare_commit(block_id, ledger_info_sigs).unwrap(); | |||
There was a problem hiding this comment.
The variable ledger_info_with_sigs is assigned on line 94, but is passed as ledger_info_sigs to prepare_commit(). Please update the argument name to match the original variable.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| @@ -124,7 +126,7 @@ where | |||
| .ledger_update(block_id, parent_block_id) | |||
| } | |||
|
|
|||
| fn pre_commit_block(&self, block_id: HashValue) -> ExecutorResult<()> { | |||
| fn pre_commit_block(&self, block_id: HashValue, sender:) -> ExecutorResult<()> { | |||
There was a problem hiding this comment.
There appears to be a syntax error in the function signature - the parameter sender: has a type annotation missing and ends with an erroneous colon. This will prevent compilation.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Description
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist