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

Move from streadway to official amqp091 go module #32

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tiggerite
Copy link
Contributor

No description provided.

@tiggerite
Copy link
Contributor Author

tiggerite commented May 24, 2022

Oops, completely missed #17 - will convert this to a draft PR instead @houseofcat so you can have it as reference for when (if?) you do the migration in v3. Would appreciate a look at my other PR though (they are separate entities), it doesn't contain breaking changes unless the Connect return type counts.

@tiggerite tiggerite marked this pull request as draft May 24, 2022 20:01
@houseofcat
Copy link
Owner

houseofcat commented May 24, 2022

There is internal behavior changes and they promise to break more API naturally.

I have to potentially rewrite the connection pool and reconnectivity.

@tiggerite
Copy link
Contributor Author

There is internal behavior changes and they promise to break more API naturally.

I have to potentially rewrite the connection pool and reconnectivity.

These are fair points. Perhaps ConnectWithErrorHandler would be better as it does not change the current API (only add a new one)? Not sure how to get around internal behaviour changes though without having a handler specifically for (re)connection.

Should I raise an issue for this, as I think having visibility of reconnection errors is important (otherwise you have no feedback when it has been successful) and being able to handle connection errors in general is a prerequisite for metrics/capturing logs with proper formatting etc?

@tiggerite
Copy link
Contributor Author

tiggerite commented May 25, 2022

@houseofcat Ok, I've reworked the PR to not break any existing APIs or internal logic, instead adding a new handler with its own creation mechanism on the connection pool, a new mechanism for creating a rabbit service with a connection pool, and reverting Connect to just return bool with a new ConnectWithErrorHandler function on connection host to handle errors (which Connect calls with no error handler). Please let me know if this is acceptable :)

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