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

Switch to wagslane RabbitMQ client #21

Open
wants to merge 6 commits into
base: rabbitmq
Choose a base branch
from

Conversation

dmcwhorter-ddl
Copy link

@dmcwhorter-ddl dmcwhorter-ddl commented Feb 22, 2023

Still have some testing to do, but this is compiling, has unit tests that pass, and has worked in simple ad-hoc tests.

wagslane golang RabbitMQ client

rabbitmq.WithConsumerOptionsQueueNoDeclare,
)
if err != nil {
c.log.Error(err, "error creating publisher")

Choose a reason for hiding this comment

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

nit: error creating consumer

)
if err != nil {
c.log.Error(err, "error creating publisher")
return nil, fmt.Errorf("error '%w' creating publisher", err)

Choose a reason for hiding this comment

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

nit consumer

if publishMandatory {
options = append(options, rabbitmq.WithPublishOptionsMandatory)
}
if publishImmediate {

Choose a reason for hiding this comment

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

this will be always false

Copy link
Author

Choose a reason for hiding this comment

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

yeah, just left the option to change this constant later

log logr.Logger
}

func createRabbitMQConnection(brokerUrl string, log logr.Logger) (ConnectionWrapper, error) {

Choose a reason for hiding this comment

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

I presume this accepts multiple hosts in brokerUrl?

Choose a reason for hiding this comment

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

if a host goes down will this seamlessly connect to another host?

Copy link
Author

Choose a reason for hiding this comment

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

i do not think it accepts multiple hosts. But i will test the failover to another host via DNS resolution/service health

Choose a reason for hiding this comment

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

how does this client support auto reconnect?

Copy link
Author

Choose a reason for hiding this comment

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

the consumer/publisher themselves use a connection from the wagslane client that reconnects if it disconnects

@dmcwhorter-ddl dmcwhorter-ddl force-pushed the rabbitmq_wagslane_client branch from 7eb2515 to 305ec27 Compare February 23, 2023 22:11
@dmcwhorter-ddl dmcwhorter-ddl requested a review from a team as a code owner May 1, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants