feat(background-processor): add BackgroundProcessorBuilder for optional components

[Anyitechs]

Fixes #3612

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[tnull]

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?

[tnull]

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?

[Anyitechs]

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.

[dunxen]

@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.

[tnull]

@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 a BackgroundProcessorBuilder with "default" fields (which might be hard in this case), and you would not have any parameters for builder().

Yeah, unclear what defaults would be.

[ldk-reviews-bot]

👋 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.

[Anyitechs]

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?

Yea, I think we can extend the new BackgroundProcessorBuilder or provide a new builder for the async variant. But extending the new builder might be the better approach as we can avoid duplicating the builder logic since the sync and async variants share the same optional components.

[tnull]

Yea, I think we can extend the new BackgroundProcessorBuilder or provide a new builder for the async variant. But extending the new builder might be the better approach as we can avoid duplicating the builder logic since the sync and async variants share the same optional components.

The question is how this would work? process_events_async is just a method that users need to call regularly, so to 'build' it with different configuration you'd need to provide multiple versions of it, which can get messy real quick, essentially exactly the thing we tried to avoid with the builder pattern. It makes me wonder if we should introduce a BackgroundProcessorParams/BackgroundProcessorConfig object that process_events_async takes as its only argument. And if we do so, maybe we should/could reuse the same object for the non-async BackgroundProcessor, also? Meaning, this object could either replace the builder or we could have the builder return the new config object.

What do you think?

[Anyitechs]

Yea, I think we can extend the new BackgroundProcessorBuilder or provide a new builder for the async variant. But extending the new builder might be the better approach as we can avoid duplicating the builder logic since the sync and async variants share the same optional components.

The question is how this would work? process_events_async is just a method that users need to call regularly, so to 'build' it with different configuration you'd need to provide multiple versions of it, which can get messy real quick, essentially exactly the thing we tried to avoid with the builder pattern. It makes me wonder if we should introduce a BackgroundProcessorParams/BackgroundProcessorConfig object that process_events_async takes as its only argument. And if we do so, maybe we should/could reuse the same object for the non-async BackgroundProcessor, also? Meaning, this object could either replace the builder or we could have the builder return the new config object.

What do you think?

Thank you for the suggestion. I agree that introducing a BackgroundProcessorConfig object would be a better option. I'm thinking we can reuse the same object for both variants and have the builder return the new config object.

What do you think about that?

[tnull]

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 std and/or futures.

[Anyitechs]

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 std and/or futures.

Yes, that can be handled conditionally using feature-gates.

I guess I can go ahead with the implementation, right?