gruelbox / transaction-outbox

Reliable eventual consistency for Microservices
Apache License 2.0
209 stars 37 forks source link

Allow TransactionOutboxImpl#scheduler and ExecutorSubmitter#executor programmatic shutdown #687

Open reda-alaoui opened 1 month ago

reda-alaoui commented 1 month ago

Fix #686

Makes TransactionOutbox and Submitter implement AutoCloseable.

Please note that if any instance is declared as a Spring bean, Spring will automatically call the close method on shutdown, which is very convenient.

badgerwithagun commented 3 weeks ago

@reda-alaoui I don't understand why the change to ExecutorSubmitter.

Currently it doesn't control the lifetime of the supplied Executor; your application does. You can easily shut your executor down without needing TransactionOutboxImpl to do it - in fact it seems quite strange to me to expect it to do so. Maybe I'm missing something?

I think the right answer for the scheduler in TransactionOutboxImpl is to go for the same approach: make it a builder option when creating the TransactionOutbox, which defaults to the same behaviour as currently.

reda-alaoui commented 3 weeks ago

@badgerwithagun,

Do you consider like me that the Submitter should be AutoCloseable ? Considering that the answer is yes, I apply the following rule: any AutoCloseable instance B created by another AutoCloseable instance A should be closed by instance A when instance A is closed. IMO, without this, apis involving AutoCloseable are not consistent and are error prone.

badgerwithagun commented 3 weeks ago

I don't see why the Executorubmitter should be autocloseable, no - it doesn't manage any resource lifetimes.

Any Executor it works with is created outside and thus should have its lifetime managed outside.

A good rule of thumb is that whatever creates a resource should close it. ExecutorSubmitter doesn't create any resources.