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

rewrite events handling to remove watermill #130

Merged
merged 6 commits into from
Aug 4, 2023

Conversation

mikemrm
Copy link
Contributor

@mikemrm mikemrm commented Jul 27, 2023

As discussed in the community call, watermill doesn't give us the necessary features we'd like to utilize with the underlying nats message. We decided to switch to using nats directly but still wanted some support for possibly changing this out later.

This rewrites events to use our own interfaces to allow for the possibility of a different event driver later.

Additionally this switches to using pull subscriptions instead of push, supports Ack, Nak and Term as well as Request/Reply semantics.

Due to the Request/Reply semantics, no longer are there separate Publisher and Subscriber configurations as the driver needs to be able to handle both.

@mikemrm mikemrm force-pushed the rewrite-events branch 22 times, most recently from 5ec2e6e to 21ff6c8 Compare July 28, 2023 19:42
events/message.go Outdated Show resolved Hide resolved
events/nats_config.go Show resolved Hide resolved
events/nats_connection.go Outdated Show resolved Hide resolved
events/nats_publish.go Outdated Show resolved Hide resolved
events/nats_publish.go Outdated Show resolved Hide resolved
events/nats_publish.go Outdated Show resolved Hide resolved
events/nats_publish.go Outdated Show resolved Hide resolved
@mikemrm mikemrm force-pushed the rewrite-events branch 2 times, most recently from 7a3f02c to dc58087 Compare July 28, 2023 20:34
@mikemrm mikemrm marked this pull request as ready for review July 28, 2023 20:58
@mikemrm mikemrm requested review from a team, nicolerenee and sfunkhouser as code owners July 28, 2023 20:58
Copy link
Contributor

@jnschaeffer jnschaeffer left a comment

Choose a reason for hiding this comment

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

More in-depth thoughts.

events/nats_config.go Outdated Show resolved Hide resolved
events/nats_config.go Outdated Show resolved Hide resolved
events/nats_config.go Outdated Show resolved Hide resolved
events/nats_config.go Outdated Show resolved Hide resolved
events/nats_config.go Outdated Show resolved Hide resolved
events/nats_publish.go Outdated Show resolved Hide resolved
events/nats_subscribe.go Outdated Show resolved Hide resolved
events/nats_subscribe.go Outdated Show resolved Hide resolved
events/nats_subscribe.go Outdated Show resolved Hide resolved
events/nats_subscribe.go Outdated Show resolved Hide resolved
events/message.go Outdated Show resolved Hide resolved
@mikemrm mikemrm force-pushed the rewrite-events branch 4 times, most recently from 29a1c2c to a7b127e Compare August 3, 2023 18:34
Copy link
Contributor

@jnschaeffer jnschaeffer left a comment

Choose a reason for hiding this comment

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

Last comments and then I think this should be good.

events/config.go Outdated Show resolved Hide resolved
events/nats_connection.go Outdated Show resolved Hide resolved
events/nats_message.go Outdated Show resolved Hide resolved
events/nats_message.go Show resolved Hide resolved
As discussed in the community call, watermill doesn't give us the
necessary features we'd like to utilize with the underlying nats message.
We decided to switch to using nats directly but still wanted some
support for possibly changing this out later.

This rewrites events to use our own interfaces to allow for the
possibility of a different event driver later.

Additionally this switches to using pull subscriptions instead of push,
supports Ack, Nak and Term as well as Request/Reply semantics.

Due to the Request/Reply semantics, no longer are there separate
Publisher and Subscriber configurations as the driver needs to be able
to handle both.

Signed-off-by: Mike Mason <mimason@equinix.com>
Signed-off-by: Mike Mason <mimason@equinix.com>
Signed-off-by: Mike Mason <mimason@equinix.com>
Signed-off-by: Mike Mason <mimason@equinix.com>
…ers to check

Signed-off-by: Mike Mason <mimason@equinix.com>
Signed-off-by: Mike Mason <mimason@equinix.com>
@mikemrm mikemrm enabled auto-merge August 4, 2023 13:16
@mikemrm mikemrm dismissed tylerauerbeck’s stale review August 4, 2023 13:23

notified tyler, changes already applied

@mikemrm mikemrm disabled auto-merge August 4, 2023 14:48
@mikemrm mikemrm enabled auto-merge August 4, 2023 14:48
@mikemrm mikemrm added this pull request to the merge queue Aug 4, 2023
Merged via the queue into infratographer:main with commit 1fb9cfe Aug 4, 2023
6 checks passed
@mikemrm mikemrm deleted the rewrite-events branch August 4, 2023 15:29
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.

7 participants