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.

Sign up for GitHub

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

Jump to bottom

Add custom filtering and exception handling to topology recovery #1312

Merged
merged 7 commits into from
Mar 21, 2023
Merged

Add custom filtering and exception handling to topology recovery #1312

merged 7 commits into from
Mar 21, 2023

Conversation

rosca-sabina
Copy link
Contributor

@rosca-sabina rosca-sabina commented Mar 14, 2023 •
edited by lukebakken
Loading

Fixes #658

Proposed Changes

This PR introduces changes that address issue #658. The changes I made are based on @acogoluegnes's suggestion of adapting the features related to topology recovery included in the Java client.

For topology recovery filtering, I added class TopologyRecoveryFilter, which is initialized at the ConnectionFactory level. It allows users to specify filters for each type of recorded entity (exchanges, queues, bindings, consumers). During topology recovery, the recorded entities will or won't be recovered based on whether they match the condition specified in the TopologyRecoveryFilter or not. If no filter is specified for an entity type, all entities of that type will be recovered.

For exception handling, I added class TopologyRecoveryExceptionHandler, which is also initialized at the ConnectionFactory level. For each type of recorded entity, it allows users to provide: a custom exception handler, which defines what action is taken in the case of an exception, and a condition, which determines what exceptions the custom exception handler applies to. If no custom exception handler is specified for an entity type, the default exception handling behavior (defined in HandleTopologyRecoveryException()) is used for all exceptions.

Types of Changes

What types of changes does your code introduce to this project?

  • Bug fix (non-breaking change which fixes issue Exception during recovery causes recovery failure #658)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

@rosca-sabina rosca-sabina force-pushed the feature/658-add-filtering-to-topology-recovery branch from 1750248 to 580014b Compare March 14, 2023 11:44
@lukebakken lukebakken self-assigned this Mar 14, 2023
@lukebakken lukebakken added this to the 6.5.0 milestone Mar 14, 2023
@lukebakken lukebakken self-requested a review March 14, 2023 17:09
@rosca-sabina rosca-sabina force-pushed the feature/658-add-filtering-to-topology-recovery branch from 580014b to b71f65d Compare March 14, 2023 18:41
@lukebakken lukebakken requested a review from Zerpet March 14, 2023 18:42
@rosca-sabina
Copy link
Contributor Author

Hello,

I noticed the Concourse CI build failed after my latest commit (I rebased from main after the model -> channel rename was merged).

Could anyone let me know if there's anything on my side I should do to get the build to pass? The build seemed to hang (it took 30 minutes), but if there are failing tests I'll take a look if you can tell me which ones failed.

Thanks!

@Zerpet
Copy link
Contributor

Zerpet commented Mar 15, 2023

Hey, thank you for opening this PR. It is in my radar to review after I finish other tasks. The Concourse build has 3 failed tests:

RabbitMQ.Client.Unit.TestAsyncConsumer.TestBasicRoundtripConcurrentManyMessages
RabbitMQ.Client.Unit.TestConnectionRecovery.TestBasicAckAfterChannelRecovery 
RabbitMQ.Client.Unit.TestConnectionRecovery.TestBasicRejectAfterChannelRecovery

Those tests succeeded in GitHub Actions, so I suspect this may be a flake in Concourse build. I re-triggered the job in Concourse.

@lukebakken
Copy link
Contributor

@rosca-sabina the Concourse build is notoriously flaky!

@rosca-sabina
Copy link
Contributor Author

Ah, I see. Thanks for re-running it!

@lukebakken lukebakken mentioned this pull request Mar 17, 2023
Closed
lukebakken
lukebakken approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

Thank you! I'll wait for @Zerpet to merge.

@rosca-sabina
Copy link
Contributor Author

Thank you for the code review and approval!

One question: After we get @Zerpet 's feedback and this is merged to main, if I provide a backport of this feature to 6.x, would it be possible for it to be released as some sort of patch to 6.4.0, like a 6.4.1?

It would really help me out to be able to use this on a project I'm working on, and it would be helpful to know if it would be feasible to have it available sooner, or if I'd have to wait for release 6.5.0 instead.

@lukebakken
Copy link
Contributor

@rosca-sabina this change requires the next version to be 6.5.0. If you'd like to open a PR now against 6.x with these changes back-ported, that would be helpful. I don't expect there to be any changes requested to this PR.

@rosca-sabina rosca-sabina requested review from lukebakken and removed request for Zerpet March 20, 2023 19:12
@rosca-sabina
Copy link
Contributor Author

I see, I'll get started on the backport then.

Also, I hit the re-review button by mistake, my bad.

@rosca-sabina rosca-sabina mentioned this pull request Mar 20, 2023
Merged
11 tasks
@rosca-sabina
Copy link
Contributor Author

And here is the backport to 6.x: #1316.

Just to make sure I understand correctly: when you say this change requires the version to be 6.5.0, do you mean that the minor version, and not the patch, needs to be incremented to respect semantic versioning? Or did you mean that this feature would have to be released as part of milestone 6.5.0, together with all of the other fixes/features that the milestone currently includes?

@lukebakken
Copy link
Contributor

do you mean that the minor version, and not the patch, needs to be incremented to respect semantic versioning?

Yes to that.

Or did you mean that this feature would have to be released as part of milestone 6.5.0, together with all of the other fixes/features that the milestone currently includes?

That would be nice, too. There are two issues I want to fix before releasing 6.5.0. I'll probably have time this week to start looking at them.

@rosca-sabina
Copy link
Contributor Author

@lukebakken I see, thanks for the clarification!

@Zerpet I just noticed my accidentally clicking re-review removed you as a reviewer. That was my bad, didn't mean to click it. Please feel free to leave your feedback when you can.

@Zerpet
Copy link
Contributor

Zerpet commented Mar 21, 2023

@lukebakken since the backport #1316 is for 6.5.0, shall we mark this PR for 7.0.0 ?

The idea is to track this enhancement in the release notes, when we decide to ship 7.0.

@lukebakken lukebakken modified the milestones: 6.5.0, 7.0.0 Mar 21, 2023
@lukebakken
Copy link
Contributor

shall we mark this PR for 7.0.0

@Zerpet good catch

@lukebakken lukebakken merged commit 857f3e7 into rabbitmq:main Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zerpet Zerpet Zerpet approved these changes

@lukebakken lukebakken Awaiting requested review from lukebakken

Assignees

@lukebakken lukebakken

Labels
None yet
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

Exception during recovery causes recovery failure
3 participants
@rosca-sabina @Zerpet @lukebakken