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

Hotel Domain with Reactor #127

Merged
merged 45 commits into from
Jan 23, 2023
Merged

Hotel Domain with Reactor #127

merged 45 commits into from
Jan 23, 2023

Conversation

bartelink
Copy link
Collaborator

@bartelink bartelink commented Dec 17, 2022

Based on Oskar Dudycz's HotelManagement sample:

  • Replaces saga/bus with Process Manager
  • Wired for MemoryStore and DynamoStore for now (PRs welcome to add more)
  • Reactor Tests
  • Reactor
  • Reactor.Integration - MemoryStore
  • Reactor.Integration - DynamoStore
  • Apply @nordfjord and @alexeyzimarev feedback
  • Make Outcome make sense
  • Review with @oskardudycz
    • should there be a version in EventSourcing.NetCore (Yes)
    • should that version also support Esdb and Mdb (Yes - the esdb stuff will be noisy, but the docker compose will make it work from the repo OOTB)
  • Add MessageDb wiring

See equinox-shipping for more extensive wiring

Base automatically changed from eqx4rc6-prp3b7 to master December 18, 2022 00:25
@bartelink bartelink marked this pull request as ready for review January 11, 2023 13:49
@bartelink bartelink changed the title Hotel WIP Hotel Domain with Reactor Jan 11, 2023
Comment on lines +29 to +38
let! residuals, fails = executeMergeStayAttempts groupCheckoutId stayIds
let events = [

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the power goes out between these two lines? each of the stays would be marked transfered, but there would be no record of that in the checkout stream. Since transfered is a final state that might pose a problem. Did I miss something?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did miss something, the decider will return the amount if it's for the same checkout

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So then the question becomes. What if the clerk receives an error, and decides to create a new group checkout in response, can we end up with two groups of checkouts? (I hate thinking about parallelism)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the group checkout owns the work, it won't be forgotten/checked off the list until that has been serviced.
All processing everywhere is and must be idempotent - as you sussed, the retry will kick out the same values next time around.
Assuming no power pulled next time, the event gets written to the group checkout, and the checkpoint moves on.

Regarding the second question re concurrency... The software should probably have an easy way / pre check to highlight group checkouts in progress for that clerk. So when they try to do one they are suggested the current one first.

Failing that, the Stay knows it has been pledged to a given checkout. So perhaps when they pick a stay and its one that's been pledged to a group, but that group one has not been closed, it can navigate you there to instead complete that. Or you can provide a merge group checkout into another group checkout function!

That's where having a reasonable UI flow defined would help more than me making stuff up!

In general though, you absolutely design for this concurrency. In a real world situation the software definitely needs to do something sensible when the inevitable happens and two parties try to do it via two different clerks 5m away from eachother. (Technically a rollback would not be hard)

The equinox-shipping sample covers the returning back of of items if you don't get to reserve all the ones you needed.

Definitely something that's easier to reason about this way than having lots of commands spread around a command queue ;)

let client = Equinox.MessageDb.MessageDbClient(writeConnStr, readConnStr)
Equinox.MessageDb.MessageDbContext(client, batchSize)
member _.MonitoringParams(log : ILogger) =
log.Information("MessageDbSource batchSize {batchSize} Checkpoints schema {schema}", batchSize, schema)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what your standard is for application of =. I've seen it a lot, but also :

Suggested change
log.Information("MessageDbSource batchSize {batchSize} Checkpoints schema {schema}", batchSize, schema)
log.Information("MessageDbSource batchSize={batchSize} Checkpoints schema={schema}", batchSize, schema)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Serilog this is de-emphasized in docs and samples

  • you've done something wrong if you're having to regex (LCD should be json rendering as opposed to text, which can be queried)
  • on console, its syntax highlighted

I do know it is sometimes neat and a pretty well established convention
But, so is not having it in all of github.com/jet !

Equinox.MessageDb.MessageDbContext(client, batchSize)
member _.MonitoringParams(log : ILogger) =
log.Information("MessageDbSource batchSize {batchSize} Checkpoints schema {schema}", batchSize, schema)
if fromTail then log.Warning("(If new projector group) Skipping projection of all existing events.")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we log this only if relevant somehow? i.e. if there's no checkpoint stored and fromTail is true

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a once-off override argument for when you make a new projection and are sure you don't want to replay, so it is not as noisy/painful as one might imagine

in Cosmos CFP, there is no API way to influence or determine a checkpoint has happened

propulsion checkpoint also provides ways to force checkpoints and/or set them to explicit values

I have not thought about the problem across all stores enough recently to have an immediate yes/no on whether that would be possible and/or desirable

(Propulsion.EventStore had a predecessor design which works more like you'd imagine - if this was to be resolved, the solution should have analogous behaviors for Propu;sion.EventStoreDb too)

@bartelink bartelink merged commit e0ca945 into master Jan 23, 2023
@bartelink bartelink deleted the hotel branch January 23, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants