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

Represent rabbit_binding:deletions() with a map instead of dict #12416

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

the-mikedavis
Copy link
Member

This is a mainly cosmetic refactor that makes the types of rabbit_binding:deletions() clearer and easier to work with.

The dict:dict() typing of rabbit_binding appears to be a historical artifact. dict has been superseded by maps. Switching to a map makes deletions easier to inspect manually and faster. Though if deletions grow so large that the map representation is important, manipulation of the deletions is unlikely to be expensive compared to any other operations that produced them, so performance is probably irrelevant.

This commit refactors the bottom section of the rabbit_binding module to switch to a map, switch the deletions() type to an opaque, eliminating a TODO created when using Erlang/OTP 17.1, and the deletion value to a record. We eliminate some historical artifacts and "cruft":

  • Deletions taking multiple forms needlessly, specifically the shape {X, deleted | not_deleted, Bindings, none} no longer being handled. process_deletions/2 was responsible for creating this shape. Instead we now use a record to clearly define the fields.
  • Clauses to catch {error, not_found} are unnecessary after minor refactors of the callers. Removing them makes the type specs cleaner.
  • rabbit_binding:process_deletions/1 has no need to update or change the deletions. This function uses maps:foreach/2 instead and returns ok instead of mapped deletions.
  • Remove undefined from the typespec of deletions. This value is no longer possible with a refactor to maybe_auto_delete_exchange_in_* functions for Mnesia and Khepri. The value was nonsensical since you cannot delete bindings for an exchange that does not exist.

@the-mikedavis the-mikedavis self-assigned this Oct 1, 2024
@the-mikedavis the-mikedavis added this to the 4.1.0 milestone Oct 1, 2024
The `dict:dict()` typing of `rabbit_binding` appears to be a historical
artifact. `dict` has been superseded by `maps`. Switching to a map
makes deletions easier to inspect manually and faster. Though if
deletions grow so large that the map representation is important,
manipulation of the deletions is unlikely to be expensive compared to
any other operations that produced them, so performance is probably
irrelevant.

This commit refactors the bottom section of the `rabbit_binding` module
to switch to a map, switch the `deletions()` type to an opaque,
eliminating a TODO created when using Erlang/OTP 17.1, and the deletion
value to a record. We eliminate some historical artifacts and "cruft":

* Deletions taking multiple forms needlessly, specifically the shape
  `{X, deleted | not_deleted, Bindings, none}` no longer being
  handled. `process_deletions/2` was responsible for creating this
  shape. Instead we now use a record to clearly define the fields.
* Clauses to catch `{error, not_found}` are unnecessary after minor
  refactors of the callers. Removing them makes the type specs cleaner.
* `rabbit_binding:process_deletions/1` has no need to update or change
  the deletions. This function uses `maps:foreach/2` instead and returns
  `ok` instead of mapped deletions.
* Remove `undefined` from the typespec of deletions. This value is no
  longer possible with a refactor to `maybe_auto_delete_exchange_in_*`
  functions for Mnesia and Khepri. The value was nonsensical since you
  cannot delete bindings for an exchange that does not exist.
@the-mikedavis the-mikedavis force-pushed the md/binding-deletions-map-refactor branch from b482ba3 to 4aa68ca Compare October 1, 2024 18:36
@the-mikedavis the-mikedavis marked this pull request as ready for review October 1, 2024 20:51
Copy link
Member

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

The elimination of functions such as anything_but/3 indeed makes the code easier to follow.

@the-mikedavis the-mikedavis merged commit 537fe75 into main Oct 2, 2024
440 checks passed
@the-mikedavis the-mikedavis deleted the md/binding-deletions-map-refactor branch October 2, 2024 19:27
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