-
Notifications
You must be signed in to change notification settings - Fork 421
feat(background-processor): introduce builder for BackgroundProcessor and process_events_async
#3688
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
feat(background-processor): introduce builder for BackgroundProcessor and process_events_async
#3688
Conversation
|
👋 I see @joostjager was un-assigned. |
tnull
left a comment
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.
Already looks pretty good, one question though.
Also, do you have any idea how we could solve the same issue for the async variant of the background processor, i.e., for the optional arguments of process_events_async?
| } | ||
|
|
||
| /// Creates a new [`BackgroundProcessorBuilder`] to construct a [`BackgroundProcessor`] with optional components. | ||
| pub fn builder<'a, PS, EH, M, CM, PGS, RGS, G, UL, L, PM>( |
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.
I'm a bit confused on the purpose of this method: in which scenario would we already have a working BackgroundProcessor to call builder on, just to then use the BackgroundProcessorBuilder to create yet another BackgroundProcessor?
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.
This is confusing indeed. I was thinking the BackgroundProcessorBuilder::new() can be an internal method and accessible via builder, but it doesn't make sense. I will remove the builder method here and allow users use BackgroundProcessorBuilder::new() directly.
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.
@tnull this doesn't take self, though?
Either way, normally the builder pattern has the builder() associated function, but works something like:
BackgroundProcessor::builder() constructs a BackgroundProcessorBuilder with "default" fields (which might be hard in this case), and you would not have any parameters for builder().
So yeah, probably removing this is best.
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.
@tnull this doesn't take self, though?
Ah true.
Either way, normally the builder pattern has the
builder()associated function, but works something like:BackgroundProcessor::builder()constructs aBackgroundProcessorBuilderwith "default" fields (which might be hard in this case), and you would not have any parameters forbuilder().
Yeah, unclear what defaults would be.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
Yea, I think we can extend the new |
The question is how this would work? What do you think? |
Thank you for the suggestion. I agree that introducing a What do you think about that? |
Sounds good to me, although we might need to account for the fact that the two variants have slightly different type requirements (i.e., trait bounds). But I think we should be able to accommodate that via feature-gates on |
Yes, that can be handled conditionally using feature-gates. I guess I can go ahead with the implementation, right? |
|
This PR is ready for another round of review. |
| onion_messenger: Option<OM>, gossip_sync: GossipSync<PGS, RGS, G, UL, L>, peer_manager: PM, | ||
| logger: L, scorer: Option<S>, sleeper: Sleeper, mobile_interruptable_platform: bool, | ||
| fetch_time: FetchTime, | ||
| config: BackgroundProcessorConfig< |
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.
nit: I think we might want to add a #[rustfmt::skip] here/above to avoid having rustfmt making this that vertical.
| } | ||
| } | ||
|
|
||
| /// Configuration for both synchronous [`BackgroundProcessor`] and asynchronous [`process_events_async`] event processing. |
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.
I'm not sure these doc links will work under all circumstances as process_events_async will only be exposed when the futures feature is set. You might need to alter the docs based on the set feature via cfg_attr, see the changes to the background processor docs in #3509 for reference how to do this.
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.
Ah true! Thank you.
| _phantom: PhantomData<(&'a (), CF, T, F, P)>, | ||
| } | ||
|
|
||
| /// A builder for constructing a [`BackgroundProcessor`] with optional components. |
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.
This should say BackgroundProcessorConfig now, no? And do we want to rename the builder to BackgroundProcessorConfigBuilder?
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.
I agree. The builder builds a BackgroundProcessorConfig object, so renaming that to BackgroundProcessorConfigBuilder might be more suitable.
| PM::Target: APeerManager + Send + Sync, | ||
| { | ||
| /// Creates a new builder instance. | ||
| pub(crate) fn new( |
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.
Seems CI is failing due to this being pub(crate)?
|
Did another round of review, will take another look once CI passes. |
| /// .build(); | ||
| /// let bg_processor = BackgroundProcessor::from_config(config); | ||
| /// ``` | ||
| pub fn from_config< |
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.
I wonder if this should even replace start (i.e., whether we should have start just take a BackgroundProcessorConfig)?
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.
I thought about this too but not sure if we want to make that change on start immediately.
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.
Okay. I think we could drop the parameters from start and really only have it start the background thread, but no strong opinion.
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.
Makes sense. So start takes a BackgroundProcessorConfig, and we can drop the from_config method?
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.
Yeah, it sounds reasoable to have that as the new default constructor, as it should be easier to use anyways. Not sure if somebody else has a different opinion though, may be good to run this by a second reviewer.
Thank you very much for the review. I'll address all the feedbacks and update the PR. |
|
🔔 1st Reminder Hey @dunxen! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @dunxen! This PR has been waiting for your review. |
|
CI still failing, fixing it. |
|
🔔 3rd Reminder Hey @dunxen! This PR has been waiting for your review. |
b967d87 to
cb7ed79
Compare
dunxen
left a comment
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.
Thanks! You'll need to run cargo fmt on lightning-background-processor/src/lib.rs and make sure each commit builds fine. You can run this check locally with ./ci/check-each-commit.sh main. It will do an interactive rebase and stop at the first problem commit, which you can fix with appropriate changes, git commit --amend '-S', and then git rebase --continue.
Thank you so much for the pointer. The email notification was getting much and I looked at the contributing guide severally to see if I can find some info on how to run the CI checks locally, but I couldn't find it. Happy to add that, if it's worth having. |
tnull
left a comment
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 seems CI is still broken due to broken docstest(s) in lightning-background-processor.
e862860 to
7e707a9
Compare
tnull
left a comment
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.
This needs a rebase now that #3509 was merged.
tnull
left a comment
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.
This unfortunately seems to need another rebase now.
joostjager
left a comment
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.
I reviewed this as is because I am coming in late as a reviewer. As a primary reviewer, I would have looked more into alternatives (#3688 (comment)). It's a lot of (duplicated) code.
| /// [`Persister::persist_manager`] returns an error. In case of an error, the error is retrieved by calling | ||
| /// either [`join`] or [`stop`]. | ||
| /// | ||
| /// This method takes a [`BackgroundProcessorConfig`] object that contains all necessary components for |
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.
Perhaps some of the docs for start are now better positioned in the config object docs?
| sweeper: Option<OS>, | ||
| logger: L, | ||
| scorer: Option<S>, | ||
| _phantom: PhantomData<(&'a (), CF, T, F, P)>, |
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.
Does this need a comment, or do Rust devs immediately understand why it is here?
| } | ||
|
|
||
| /// Builds and returns a [`BackgroundProcessorConfig`] object. | ||
| pub fn build( |
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.
Maybe not doing the builder and just having a config object is worth it to cut down on all the generics and duplication?
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.
But without the builder we’d be back at having the user construct all the objects manually, and also trying to construct a suitableNone value for some of the fields, no?
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.
I mean don't require the build call to copy the object into something immutable. Just use the builder (then not a builder anymore) as the config object itself.
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.
I mean don't require the
buildcall to copy the object into something immutable. Just use the builder (then not a builder anymore) as the config object itself.
Hmm, I'd rather not. The builder pattern is a very common pattern in Rust, and everybody 'knows how it works'. Plus, it of course has the benefit that you don't have to copy anything: as build takes self, move semantics apply and the values are moved to the new object.
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.
Ah, wrong on the copy part. Also no perf consequence obviously here.
But just leaving out .build() isn't that hard? It also isn't as if this config needs to be used in many places. Just once per project. To me all the duplication is already bad enough as it is, and getting rid of some by deviating a bit from a pattern is worth it to me.
Interested to hear @Anyitechs's opinion.
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.
But just leaving out
.build()isn't that hard?
This is part of the builder pattern. Without it we Rust devs would be very confused and running in circles tearing out our hair not knowing how to proceed.
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 also isn't as if this config needs to be used in many places. Just once per project. To me all the duplication is already bad enough as it is, and getting rid of some by deviating a bit from a pattern is worth it to me.
Interested to hear @Anyitechs's opinion.
I understand your concerns on the duplication, @joostjager. An improved version to help with the duplication would've been to reuse the same config object for the sync and async variant, but that wasn't possible because of the type difference on the trait bounds on some of the components (the EventHandler for example, attempted to handle that via feature-gates but it wasn't successful).
But just leaving out .build() isn't that hard?
I think leaving out .build() will make the API a bit complicated to use.
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.
I am okay then with leaving the builder pattern in, but can you explain me why leaving out .build() makes it complicated?
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.
but can you explain me why leaving out .build() makes it complicated?
I mean leaving it out makes things unclear and users may be unsure when the configuration is complete and it's object ready to use (some users might try to use the builder as the final object). It's conventional to have a .build() method when using the builder pattern as this is what gives the "We're done configuring" signal and allows for final validation.
|
@Anyitechs let us know when you rebased and this is ready for another (hopefully final) look! |
be1bdcf to
b86b834
Compare
b86b834 to
2b83ad2
Compare
Thanks! This PR is ready for another look. Rebased and pushed 2b83ad2 to address @joostjager's comment on the docs update (will squash it in if the changes looks good). |
|
🔔 1st Reminder Hey @tnull @joostjager! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @tnull @joostjager! This PR has been waiting for your review. |
tnull
left a comment
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.
Code changes basically LGTM. I also validated that this will work with LDK Node.
However, I unfortunately just realized we currently still require the user to always provide values, even those who are/should be optional.
For example, if I just remove one with_ call from tests:
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index eb0cbbf1e..6264746c2 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -2947,7 +2947,6 @@ mod tests {
builder
.with_onion_messenger(Arc::clone(&nodes[0].messenger))
.with_scorer(Arc::clone(&nodes[0].scorer))
- .with_liquidity_manager(Arc::clone(&nodes[0].liquidity_manager))
.with_sweeper(Arc::clone(&nodes[0].sweeper));
let config = builder.build();we still get the following error:
error[E0283]: type annotations needed for `BackgroundProcessorConfigBuilder<'_, ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., _, ..., ..., ..., ..., ..., ...>`
--> lightning-background-processor/src/lib.rs:2937:7
|
2937 | let mut builder = BackgroundProcessorConfigBuilder::new(
| ^^^^^^^^^^^ -------------------------------- type must be known at this point
|
= note: the full type name has been written to '/home/tnull/workspace/rust-lightning/target/debug/deps/lightning_background_processor-4ef49ba2fa37c38a.long-type-17867618020029664474.txt'
= note: cannot satisfy `_: Deref`
note: required by a bound in `BackgroundProcessorConfigBuilder`
--> lightning-background-processor/src/lib.rs:1660:16
|
1639 | pub struct BackgroundProcessorConfigBuilder<
| -------------------------------- required by a bound in this struct
...
1660 | LM: 'static + Deref + Send,
| ^^^^^ required by this bound in `BackgroundProcessorConfigBuilder`
help: consider giving `builder` an explicit type, where the type for type parameter `LM` is specified
|
2937 | let mut builder: BackgroundProcessorConfigBuilder<'_, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, LM, _, _, _, _, _, _> = BackgroundProcessorConfigBuilder::new(
| ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
For more information about this error, try `rustc --explain E0283`.
error: could not compile `lightning-background-processor` (lib test) due to 1 previous error
exit 101
But the main motivation for this change in the first place is to provide users a more reasonable API that allows them to not provide all fields, no? I think this means we'd still need to provide internal dummy values, and have them only overridden with the with_ methods, as otherwise the changes in this PR just add a bunch boilerplate without fixing the actual issue we wanted to address.
Could you look into this and see whether we find a way forward here? While we do this, could we also align the GossipSync case, which already did it with a dummy value before and hence was somewhat the odd one out in the new builder API?
Sorry for discovering this only now.
Thank you for catching this. I'm working on a fix already. |
|
If we still need dummy implementations for everything, I'd feel more strongly than before about just dropping the builder. |
Hmm, I imagine the big difference would be that they'd still be completely internal and hidden from the API. I.e., the user can just use the builder as expected, without having to know/looking up all the parameters and learning how to construct a dummy value for each of them. Rather, if they are necessary at all, the builder would just use them internally, until the user explicitly overrides them. |
|
I don't think it is worth all the extra code and duplication. Especially because this is a single high-level call and not something that is used everywhere. |
|
🔔 2nd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
@Anyitechs Any news here? Do you still intend to finish this PR? |
Yes, please pardon the delay. I'll be moving things forward on this PR this weekend, and should have an update by next week. |
|
Closing this PR as attempts to fix the issue using the builder method and other options didn't fully work (see this). Attempted to do a dynamic dispatch but some of the traits used by the optional components, e.g Another attempted option was to use a No-operation implementation for the traits so it can be used as defaults (or dummy value) when the user doesn't provide the optional fields. But the approach is too verbose because of the associated types on the traits that goes in deep and require the trait items are also implemented. For example, having a noop implementation for the Also, I explored the typestate/state machine builder pattern, and while being verbose and complex, it still didn't work as it requires a concrete type during state transitions. I'm happy to get back to this if there's another way to go around this issue to provide users a more reasonable API that allows them to not provide all fields. |
This PR introduces two main improvements to the
BackgroundProcessorandprocess_events_async:Adds a
BackgroundProcessorConfigBuilderandBackgroundProcessorConfigAsyncBuilderfor a more ergonomic way to construct aBackgroundProcessorand it's async variant (process_events_async), and supports optional parameters through builder methodsIntroduces
BackgroundProcessorConfigandBackgroundProcessorConfigAsyncto standardize configuration for the sync (BackgroundProcessor) and async (process_events_async) variant. The Builder returns this config object, which can be used to start the event viaBackgroundProcessor::startandprocess_events_asyncFixes #3612