Skip to content
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

[Host.Outbox] Support for UUIDv7, different message keys and refactor #315

Merged
merged 1 commit into from
Nov 3, 2024

Conversation

zarusz
Copy link
Owner

@zarusz zarusz commented Oct 3, 2024

Several changes to SlimMessageBus.Host.Outbox.* in preparation for onboarding other storage providers:

  • IGuidGenerator in SlimMessageBus.Host
  • Default GuidGenerator registered in DI
  • Ability to register custom UUIDv7 implementation based on net9.0 or 3rd party library before net9.0
  • In the .Outbox.Sql layer the ability in the SqlOutboxSettings to
    • specify client ID generation or database ID generation (GUIDs),
    • specify GuidGeneratorType in the MSDI or GuidGenerator instance for the client ID generation.
  • Renamed SlimMessageBus.Host.Outbox.DbContext to SlimMessageBus.Host.Outbox.Sql.DbContext - to express its bound to MSSQL
  • I've moved the "system fields" from OutboxMessage down to the .Outbox.Sql layer into a specialized type SqlOutboxMessage.
  • The PK for an outbox message is now a generic, and so we should allow for other types than Guid as the PK. .Outbox.Sql will still use the Guid for backward compatibility

Closes #311

@zarusz
Copy link
Owner Author

zarusz commented Oct 4, 2024

@dundich here is another iteration:

  • IOutboxMessageFactory to be able to create messages (and store), Id/Timestamp assignment happens in the implementation. It takes only the minimal params in and returns the outbox message Id.
  • IOutboxMessageRepository does not have the Save method anymore - in favor of the above
  • SqlOutboxMessageRepository is the implementation of the above two interfaces (in the .Host.Outbox.Sql layer), currently tied to MSSQL but could get a postgres dialect
  • SqlDialect - see this setting, it will allow to specify the dialect, just added Postgres

There is some more work needed to

  • drive by the OutboxSettings configuration to inform what a concrete implementation of IGuidGenerator to resolve from MSDI - this will be the main bit that will allow to generate the uuidv7 for you (either net9 or plugging a 3rd party lib like NewId for < net9.
  • document how to inject different guid generator for uuidv7 case

Copy link

sonarcloud bot commented Oct 4, 2024

@dundich
Copy link
Contributor

dundich commented Oct 5, 2024

I agree it is worth defining the API for connecting Outbox strategies.

But before Api I think it is necessary to define possible interception points for working with OutboxFlow. So far I see them in three places:

Interception points

  1. current (classic) mode : first outbox -> transport (https://microservices.io/patterns/data/transactional-outbox.html)
  2. or optimistic mode : first transport -> if error -> outbox ([Host.Outbox] optimistic mode - hybrid strategy #316)
  3. CircuitBreaker - if no check -> outbox #251 Health check circuit breaker #282

They should probably be set up through configuration.

Hybrid outbox ?

like as (https://github.com/zarusz/SlimMessageBus/blob/master/docs/provider_hybrid.md) for (mssql, pg, redis)

I mean connect multiple strategies and then bind them via configuration

but imho overhead - maybe it's not worth it yet and with an eye to the future.?

Describe the prototype connection API

like as #282 (comment)

note: I'm sorry I can't look at the code yet..

ps

Perhaps then it will become clearer whether we need additional abstractions.

@zarusz
Copy link
Owner Author

zarusz commented Oct 5, 2024

@dundich all great ideas, but that needs to be done in stages.
I don't want this PR to snow ball into many new features.

The intent of this PR is to introduce a way to inject the Guid generator and connect the existing time provider interface into outbox. This is to enable your need for UUIDv7.

Any further enhancements need to come after that.

Next stages could include:

Let me know if you have other priorities in mind.

@zarusz
Copy link
Owner Author

zarusz commented Oct 5, 2024

As for the hybrid outbox my thinking is this:

  • Any other SQL database (e.g. Pg, MsSql, MySql) could simply be a dialect setting (on the SlimMessageBus.Host.Outbox.Sql layer with necessary adjustments).
  • When we need to go to leveraging let say Redis for storing outbox message that it simply be another layer I am thinking (SlimMessageBus.Host.Outbox.Redis).

Also, the idea with SlimMessageBus.Host.Outbox layer is that it should contain enough generic logic for any type of outbox storage used.

Not sure that you'd need a hybrid outbox with both some messages routed to SQL outbox, others to Redis Outbox.
Unless this is what you have good use case for and need? If so outbox-es could be named (yes like in the hybrid) and then routed from the publish outbox interceptor to the right outbox.

Again, we need to do this in stages :)

@dundich
Copy link
Contributor

dundich commented Oct 5, 2024

I agree with all of this - the elephant needs to be eaten in pieces.
As for the hybrid outbox - unnecessary engineering, I don't think it will be useful.

As for the dialect, I didn't quite catch the point. I thought it was enough to register my IPgSqlRepository implementation via config... and why I need a dialect, I don't understand yet.....

it seems to me that you want to put everything in one SlimMessageBus.Host.Outbox.Sql or am I wrong?

ps.

To get started on Pg I don't like Mssql yet. Do I understand correctly that we get rid of it, leaving IConnection ITransaction abstractions without vendors?

image

@zarusz
Copy link
Owner Author

zarusz commented Oct 6, 2024

As for the dialect, I didn't quite catch the point. I thought it was enough to register my IPgSqlRepository implementation via config... and why I need a dialect, I don't understand yet.....

it seems to me that you want to put everything in one SlimMessageBus.Host.Outbox.Sql or am I wrong?

Oh yes I see we would need a new project SlimMessageBus.Host.Outbox.PqSql that references NpgSql, that injects a new ISqlOutboxMessageRepository (or IOutboxMessageRepository) implementation. I thought we could use the SqlClient already present and just via a SqlDialect property control which SQL scripts are used for MSSQL or Postgres - but that is not the case.

@zarusz zarusz changed the base branch from master to release/v3 October 6, 2024 13:08
@zarusz zarusz force-pushed the feature/uuidv7 branch 2 times, most recently from 3ee000f to f0a2799 Compare October 6, 2024 19:51
@zarusz zarusz marked this pull request as ready for review October 6, 2024 20:02
@zarusz zarusz force-pushed the feature/uuidv7 branch 3 times, most recently from 82ef2b8 to 2a52846 Compare October 9, 2024 20:12
@zarusz
Copy link
Owner Author

zarusz commented Oct 9, 2024

@dundich I am ready with all my changes. Hopefully, this will give a baseline for your Postgress Outbox work (although some more refactoring might be needed). Certainly, it will allow for Guid generation strategies e.g. UUIDv7.

@dundich
Copy link
Contributor

dundich commented Oct 10, 2024

Refactoring - need to understand how best to reorganize two projects with mssql:

  1. Outbox.Sql
  2. Sql.Common

#315 (comment)

ps.
I think a separate ticket is required for this ... and I can start (draft) refactor or contract implementation if you have other priorities.

@dundich
Copy link
Contributor

dundich commented Oct 10, 2024

@dundich I am ready with all my changes. Hopefully, this will give a baseline for your Postgress Outbox work (although some more refactoring might be needed). Certainly, it will allow for Guid generation strategies e.g. UUIDv7.

note: There is one point - since the contract has been rewritten, now id generation may come with a response from Pg or Mssql, the generator may be useless here (uid_generate_v1mc() or NEWSEQUENTIALID (Transact-SQL) orderable by time - guid ).

but he'll still be useful for nosql ... or saga in future

@zarusz
Copy link
Owner Author

zarusz commented Oct 10, 2024

Refactoring - need to understand how best to reorganize two projects with mssql:

  1. Outbox.Sql
  2. Sql.Common

#315 (comment)

ps. I think a separate ticket is required for this ... and I can start (draft) refactor or contract implementation if you have other priorities.

I think at this point I would just start with a new project (e.g. SlimMessageBus.Host.Outbox.PgSql or Outbox.PostgreSql) and see how it goes. Ideally if you can keep is such that SlimMessageBus.Host.Outbox has some shared logic between MsSql and Posttgres implementations.

The SlimMessageBus.Host.Sql.Common is an attempt to share any MSSQL aspects between the MSQL outbox and a new SQL transport I have started (SlimMessageBus.Host.Sql). Not sure that you need to make much or any changes to SlimMessageBus.Host.Sql.Common. However, you will see once you start the feature.

@dundich I am ready with all my changes. Hopefully, this will give a baseline for your Postgress Outbox work (although some more refactoring might be needed). Certainly, it will allow for Guid generation strategies e.g. UUIDv7.

note: There is one point - since the contract has been rewritten, now id generation may come with a response from Pg or Mssql, the generator may be useless here (uid_generate_v1mc() or NEWSEQUENTIALID (Transact-SQL) orderable by time - guid ).

but he'll still be useful for nosql ... or saga in future

Yes, that is true. I wasn't aware of the new NEWSEQUENTIALID. Makes sense to also have that as an option on the Outbox.Sql. Let me see if perhaps this is worth adding to my PR! Then on the postgree side you could adopt a similar approach/setting.

@zarusz zarusz force-pushed the feature/uuidv7 branch 4 times, most recently from 328add6 to eb6bd33 Compare October 11, 2024 20:45
@zarusz
Copy link
Owner Author

zarusz commented Oct 15, 2024

@dundich I am debating whether not to refactor this Outbox.Sql and Outbox.DbContext even further
The EntityFrameworkCore could act as the database abstraction to any type of SQL database (MS SQL, and Postgres) by bringing in

There are still a few considerations around managing transactions

The only caveat is if we let EF Core to manage the outbox messages it might end up being slower than the current T-SQL implementation.

cc: @EtherZa (in case wanted to weigh in)

@EtherZa
Copy link
Contributor

EtherZa commented Oct 16, 2024

@zarusz I would be apprehensive to create a single plugin that supports multiple database providers.

There are slight variations in the dialects that would result in a number of conditionals (non-clustered indexes in sql but not in postgres or mysql, select top vs limit, etc). Further to this, there are features that can be leveraged in some providers that are not available in others (postgres notficiations). As such, you will get a far better implementation with a tailored solution.

As for using EF; you run the risk of pinning the EF version which would be a deal breaker for me personally. It would also reduce the feature set to the lowest common denominator.

I agree with @dundich that the implementation/DTO could be cleaned up. The provider specific implementation can be hidden from SlimMessageBus.Host.Outbox - this could even include the type of id being used (performance would be improved in the MS SQL implementation if a sequence were to be used instead of a GUID).


As a side note (and definitely scope creep for this discussion); the current implementation could be vastly improved if the message processing were to be partitioned by path (and subscription for ASB). Processing by partition (round-robin, instead of purely by sequence) would ensure that multiple queues are populated instead of just one working through a backlog.

ie. In the workflow A => B, where A enqueues a message for B; if 100k messages are outboxed for the A queue, the current implementation will sequentially run through the messages adding all 100k to A before starting to add anything to the B queue. The B consumers then sit idle while A works its way through the backlog.

I only bring this up as it could be benefitial to keep in mind if refactoring is planned.

@dundich
Copy link
Contributor

dundich commented Oct 16, 2024

I would like to join right now, but I am busy and so I will keep it short

I am also against linking to EF. I want to use PG features such as: partitions, skip-locked

I would focus on contract to connect different implementations and customize them.

  1. Definition of DTOs (sys fields)
  2. Ability to save as batch (e.g. for buffered flush)
  3. Configuration (base, specific for implementation)
  4. etc.

ps. then connect MSSQL (as a template for others)

@dundich
Copy link
Contributor

dundich commented Oct 16, 2024

Again, I think the problem lies in the name - repository.

It is clear that a repository should be as simple as possible, hence the desire to have one BLL with single settings. I think this is a mistake.

We should allow each vendor to have their own properties and define Outbox logic independently, just like Buses have their own specifics..

e.g. the vendor's use of 3NF or lowcardinality for the fields: BusName , MessageType, Path, InstanceId

@zarusz
Copy link
Owner Author

zarusz commented Oct 16, 2024

@dundich and @EtherZa thanks for your perspectives and ideas.

The idea with EF Core serving as an abstraction would be an addition to ensure broader databases coverage. So let say we would still have a MSSQL (like today) and PG specific plugin (hopefully built by @dundich), but any other db would fallback to a slower EF implementation. I share the same over abstraction and lower performance concern in contrast when comparing EF approach vs optimized Outbox.Sql or PQ.

Existing ones that we'd preserve in v3.0.0

SlimMessageBus.Host.Outbox (shared abstracted outbox logic with IOutboxMessageRepository)
SlimMessageBus.Host.Outbox.Sql (specific to MSSQL)
SlimMessageBus.Host.Outbox.Sql.DbContext (depends on SlimMessageBus.Host.Outbox.Sql, renamed from SlimMessageBus.Host.Outbox.DbContext

The new ones I can foresee:

SlimMessageBus.Host.Outbox.Postgres (specific to Postgres, ideally dependant on SlimMessageBus.Host.Outbox to drive re-use)
SlimMessageBus.Host.Outbox.DbContext (a new one that uses EntityFrameworkCore as the abstraction, slower)
SlimMessageBus.Host.Outbox.Memory (an in memory buffer to postpone the publish of messages until unit of work finish / tx.Commit)

@EtherZa the partitioning feature on MSSQL is a good one, I will capture as another feature request.

@dundich again, the repository is a way to delegate the db specific SQL dialect (e.g. Outbox.Sql / Outbox.Pg) and hopefully for the db specialized plugins to still leverage the common logic within Host.Outbox. We could rename to OutboxService or something if that sits better. Now, we tried to move the PK out of the entity to allow for provider specific Key types, I could still try to revive that. My stance on the abstraction in Host.Outbox - if its is not good enough for Pg implementation this is where I think (once you get there) make a decision to refactor the Host.Outbox layer some more or part ways from it and make the Pg completely custom (I don't think there would be 0 reuse between MSQL and Pg).

@dundich
Copy link
Contributor

dundich commented Oct 16, 2024

Rather, you're right in that the DBs have common specifics.

Please consider a few points:

  • create([]) // to buffer items and then bulk copy to DB - where speed is critical
  • LockAndWriteBuffer() // allocate buffer for writing I'm not sure
  • OutboxMessage // hide specific fields - keep only general

@zarusz zarusz changed the title Support for UUIDv7 values [Host.Outbox] Support for UUIDv7, different message keys and refactor Oct 16, 2024
@zarusz
Copy link
Owner Author

zarusz commented Oct 19, 2024

@dundich, I think I am now done with my changes.

  • I've moved the "system fields" down to the .Outbox.Sql layer into a specialized type SqlOutboxMessage.
  • The PK is now a generic, and so we should allow for other types than Guid as the PK. .Outbox.Sql will still use the Guid
  • But .Outbox.Sql will be able to choose the Guid generation strategy - I've moved the strategy settings over to that layer too. If you'd need something similar in Postgress, bring it down to .Outbox, but I sense you will go down a different route more optimal towards PG.
  • I will have my other exploratory feature branch open (in memory outbox + abstraction using EntityFrameworkCore) and perhaps evolve it depending on time and priority.

Happy to merge, unless you see anything out of line.

@dundich
Copy link
Contributor

dundich commented Oct 21, 2024

pls look at small fix for uuidv7 - #325

@zarusz
Copy link
Owner Author

zarusz commented Oct 21, 2024

Yes, good idea! Merged.

… flexible PK types, refactor

Signed-off-by: Tomasz Maruszak <[email protected]>
@zarusz
Copy link
Owner Author

zarusz commented Nov 3, 2024

@dundich I managed to stabilize the Outbox tests. There is more work to be done in the near future but will park this for now under #329 to allow you to go forward with the Postgress work. I have few other priorities too emerging now.

@zarusz zarusz merged commit ee36ea4 into release/v3 Nov 3, 2024
4 checks passed
@zarusz zarusz deleted the feature/uuidv7 branch November 3, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants