Add RabbitMQ-based Implementation of EventPublisher Trait #53
Conversation
G8XSU
force-pushed
the
2025-03-12-rabbitmq-impl
branch
from
64989d9 to
ddf96fa
Compare
|
Rebased on main after #51 merged. |
ldk-server/Cargo.toml
Outdated
| # Required for RabittMQ based EventPublisher. Only enabled for `events-rabbitmq` feature. | ||
| lapin = { version = "2.5.1", optional = true } |
There was a problem hiding this comment.
Looks like this is require a change of the MSRV to 1.81.0 with the feature enabled.
There was a problem hiding this comment.
Even though I can fix the msrv to 1.75 for this commit,
Fwiw, longer term, I don't think we can make a commitment to msrv when these optional features are enabled.
This is because most of these deps and rust ecosystem doesn't follow the same msrv principle.
rustls recently bumped their msrv due to which ldk-node, bdk and everyone had to bump theirs.
aws sdk for rust doesn't follow a conservative msrv, and older versions risk being incompatible with apis.
Might need to come up with a separate msrv policy when optional features are enabled.
fixed msrv build for 1.75. for now.
| let event_publisher: Arc<dyn EventPublisher> = { | ||
| let rabbitmq_config = RabbitMqConfig { | ||
| connection_string: config_file.rabbitmq_connection_string, | ||
| exchange_name: config_file.rabbitmq_exchange_name, | ||
| }; | ||
| Arc::new(RabbitMqEventPublisher::new(rabbitmq_config)) | ||
| }; |
There was a problem hiding this comment.
Should we open a connection to ensure the config parameters are valid?
There was a problem hiding this comment.
that's a good question.
I was doing that earlier, then moved to lazy connection initialization. Opening a connection in construction of RabbitMqEventPublisher makes it fallible, it has its pros and cons.
It is mainly about : should we fail ldk-server startup if we can't reach rabbitmq?
In case of production deployments, should deployments of ldk-server fail if a dependency service is experiencing an outage?
Argument for non-fallible: we should keep deployments of different systems decoupled.
Argument for fallible: we check availability and correctness of endpoint upfront.
I tend to favor the non-fallible approach since rabbitmq unavailability shouldn't create a ldk-server outage(in case of restarts/deployments etc.). Even with event-publishing degraded, uptime of lightning node is important.
There was a problem hiding this comment.
Makes sense, though how would an operator be aware of the rabbitmq unavailability? IIUC, unavailability would cause the event queue to be backed up. The underlying node would still progress, but from the user's perspective the payment store would be stale since event publishing failure would prevent persisting the payment.
This doesn't need to hold up the PR, but just wondering how this would be monitored.
There was a problem hiding this comment.
We are going to have metrics for this and persistence failures, ideally those should be monitored and alerted on. We can configure pages on metrics, that way we don't have to shutdown the service to bring attention.
Maybe we can also configure some threshold(configurable) at which the process will shutdown instead of accumulating more backlog.
G8XSU
force-pushed
the
2025-03-12-rabbitmq-impl
branch
from
661aa18 to
368c398
Compare
G8XSU
force-pushed
the
2025-03-12-rabbitmq-impl
branch
from
38d271b to
b11e719
Compare
|
Rebased due to merge conflicts with main and squashed current fixups as well. |
RabbitMQ was selected because it is one of the most robust and battle-tested open-source messaging systems, allowing for durable, buffered messages and flexible exchange options. This ensures that events are not lost, even in the case of restarts, network failures, or consumer failures, while keeping consumers decoupled. * Feature-based: Only enabled if we enable `events-rabbitmq` feature, this allows us to support multiple implementations and no strict coupling with a single impl while keep our dependencies to the minimum. * Durable Publishing: Uses publisher confirms and persistent delivery mode to guarantee events are stored by RabbitMQ before returning Ok(()). * Lazy Initialized connection: Wraps Connection and Channel in Option for lazy connection init via ensure_connected. * Reconnection Handling: Implements a single reconnect attempt per publish call. Event publishing is already retried, so this keeps it simple for now.
G8XSU
force-pushed
the
2025-03-12-rabbitmq-impl
branch
from
b11e719 to
d6c3618
Compare
|
Fixed and squashed fixup. |
2 participants
Note: It is only enabled if we enable
events-rabbitmqfeature.RabbitMQ was selected because it is one of the most robust and battle-tested open-source messaging systems, allowing for durable, buffered messages and flexible exchange options.
This ensures that events are not lost, even in the case of restarts, network failures, or consumer failures, while keeping consumers decoupled.
events-rabbitmqfeature, this allows us to support multiple implementations and no strict coupling with a single impl while keep our dependencies to the minimum.Depends on #51
As part of #37