Skip to content

Conversation

greged93
Copy link
Collaborator

@greged93 greged93 commented Oct 13, 2025

closes: #319

@greged93 greged93 requested a review from frisitano October 13, 2025 16:22
Comment on lines 644 to 645
async fn get_n_l1_messages(
&self,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a stream which captured the lifetime of the TX caused lifetime issues when trying to return the stream from the database call.

Copy link

codspeed-hq bot commented Oct 13, 2025

CodSpeed Performance Report

Merging #358 will improve performances by 34.88%

Comparing feat/update-retry (c6cbd77) with main (50b63b2)

Summary

⚡ 1 improvement
✅ 1 untouched

Benchmarks breakdown

Benchmark BASE HEAD Change
pipeline_derive_in_file_blobs 972.4 ms 721 ms +34.88%

@greged93 greged93 requested a review from jonastheis October 14, 2025 16:17
Copy link
Collaborator

@frisitano frisitano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Added some minor comments inline, primiarly about when we should use the raw methods and when to instantiate a transaction.

@greged93 greged93 changed the title feat: update retry mechanism using tower::Service feat: update retry mechanism Oct 16, 2025
frisitano
frisitano previously approved these changes Oct 16, 2025
Copy link
Collaborator

@frisitano frisitano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@greged93
Copy link
Collaborator Author

@frisitano should we merge this with the old benchmark and fix in another PR?

@frisitano
Copy link
Collaborator

@frisitano should we merge this with the old benchmark and fix in another PR?

Yes lets revent back to the old bencmarks, get CI green and merge. We can look at codespeed in another PR as you suggest.

@greged93
Copy link
Collaborator Author

@frisitano regression seems to have came from not having a get_n_block_data_hint. With the added method in 22e5d4c, regression is corrected.

@frisitano
Copy link
Collaborator

frisitano commented Oct 19, 2025

@frisitano regression seems to have came from not having a get_n_block_data_hint. With the added method in 22e5d4c, regression is corrected.

Can you elaborate on this please? In main we don't have a get_n_block_data_hint so why would this result in a regression?

Also looks like CI isn't passing

edit: Is this because in the derivation pipeline using the new api we would create a new transaction for each block data hint we fetched?

@frisitano frisitano self-requested a review October 19, 2025 11:03
Copy link
Collaborator

@frisitano frisitano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Chain Orchestrator] Retry on failure

2 participants