refactor: ntx builder start up#1610
Conversation
SantiagoPittella
force-pushed
the
santiagopittella-ntx-builder-initialization
branch
from
0cc662b to
7d9fdb9
Compare
SantiagoPittella
force-pushed
the
santiagopittella-ntx-builder-initialization
branch
from
7d9fdb9 to
bbe4d47
Compare
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Looking good; will do another pass next week
| @@ -77,6 +78,20 @@ pub struct NtxBuilderConfig { | |||
| pub script_cache_size: NonZeroUsize, | |||
There was a problem hiding this comment.
This is more a thought experiment, but I wonder if we ever want to set this to zero? Seems unlikely, but that would effectively be the same as disabling it.
I think NonZero is used because that's what LRU requires, so we would have to do struct MyLru<T>(Option<Lru<T>>) which is a bit more work but not too much.
I kind of like the idea that we can disable caching e.g. for performance testing. wdyt? As a separate issue though, if anything. Could also easily be a YAGNI thing so maybe we just leave as is until we do need it.
There was a problem hiding this comment.
Maybe to test performance of the underlaying implementation? Otherwise I don't see why we need that
|
|
||
| impl NtxBuilderConfig { | ||
| /// Converts this CLI config into the ntx-builder's internal config. | ||
| pub fn into_builder_config( |
There was a problem hiding this comment.
Bit confusing with the stuttering builder/builder naming, but that's not this PRs fault. We should come up with something better than network transaction builder..
Maybe network transaction..
- producer
- service
- component (bleh)
- handler
- .. hm.
There was a problem hiding this comment.
Yeah, I agree. We tend to use a lot the builder pattern so it is a bit confusing. I like both "producer" and "service", and even "consumer". I'll create a separate issue to discuss and rename the component since it touches too many files for this PR
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Mirko-von-Leipzig
deleted the
santiagopittella-ntx-builder-initialization
branch
3 participants
closes #1573
Refactors NTX Builder startup into distinct phases and introduces typed configuration.
NetworkTransactionBuilderinitialization phasesNtxBuilderConfigstructRegarding the new
InitializedBuilder:The
initialize()method creates a gRPC subscription stream that needs to be stored for later use inrun(). Rust'simpl TryStreamreturn types are opaque, causing lifetime errors when stored in a struct. Boxing the stream with Pin<Box<dyn Stream + 'static>> forces it to own its data, but this requires a separate struct to hold the boxed stream between initialization and execution. Couldn't thought of anything better cc @Mirko-von-Leipzig