BrighterCommand/Brighter

Bug: DynamoDbOutbox Archiver Missing Topics

Closed this issue · 8 comments

Describe the bug

Current implementation depends on DynamoDbOutbox._topicName being populated as messages are added to the outbox. The archiver is trying to find messages through the async outbox but the sync version is used when adding to the outbox.

services
    .AddBrighter()
    .UseExternalBus(...)
    .AutoFromAssemblies()
    .UseDynamoDbOutbox() <---- registers 3 instances of the outbox, IAmAnOutbox, IAmAnOutboxSync, IAmAnOutboxAsync
    .UseOutboxSweeper() <---- uses non-async outbox
    .UseOutboxArchiver(new NullOutboxArchiveProvider()); <---- uses async outbox

Proposed fix

Short term
Make DynamoDbOutbox._topicNames static

Long term
Maybe it should be possible to derive topic names from publications so that the archival process can run without needing any messages added first

Further technical details

  • Brighter v9

I can't see a good reason for having three instances of the outbox registered, so to me the most sensible option would be to ensure only a single instance of the dynamo outbox is registered for all three interfaces.

One possible issue there is that if you create a DynamoDbOutbox instance inside UseDynamoDbOutbox and add that instance directly, then the method can't be called until IAmazonDynamoDB has been registered, instead of just relying that it's going to be registered at some point. So perhaps, then, BuildDynamoDbOutbox should, instead of automatically creating a new outbox instance, try to retrieve the implementation of IAmAnOutbox and then return that instance if it exists. Otherwise return a new instance.

Maybe it should be possible to derive topic names from publications so that the archival process can run without needing any messages added first

In V10 we have the Topic on the Publication, we could backport this to V9, as it is additive and not a breaking change.

I can't see a good reason for having three instances of the outbox registered, so to me the most sensible option would be to ensure only a single instance of the dynamo outbox is registered for all three interfaces.
Agreed, that is an error. It implements all three for us.

BuildDynamoDbOutbox should, instead of automatically creating a new outbox instance, try to retrieve the implementation of IAmAnOutbox and then return that instance if it exists. Otherwise return a new instance.
Yeah, it sounds reasonable that we don't rebuild if we have built it once already. Effectively it is a singleton. In fact, ExternalBusService pretty much enforces that.

@dhickie @iancooper I've raised #3358 as suggested

@jtsalva Approved and merged. Do we need to fix this in V9 as well?

@iancooper the merged PR was for v9. I had a look at the v10 master branch and I can't find UseDynamoDbOutbox there

OK, I'll close