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 Mnesia-specific code to rabbit_db* modules #6430

Merged
merged 7 commits into from
Dec 14, 2022

Conversation

dumbbell
Copy link
Member

@dumbbell dumbbell commented Nov 18, 2022

The idea is to isolate in those modules the code to handle the records in the database. For now, we only support Mnesia, but there is work in progress to move to another database engine. Doing this reorganization of the code will also isolate the changes of this upcoming database engine switch, making them easier to rebase and review.

In the end, the rabbit_db* modules should export functions for each operations we want to perform. Things like "add this record", "list records matching that", "update a record using an anonymous function inside a transaction", etc.

Only the following subsystems are impacted in this commit:

  • virtual vhosts
  • internal users
  • runtime parameters

They were modified together because they depend on each other.

It is a preparation step to be able to change the database layer.

@dumbbell dumbbell self-assigned this Nov 18, 2022
@dumbbell dumbbell force-pushed the reorganize-code-for-khepri-phase1 branch 3 times, most recently from e2486e4 to 41627e0 Compare November 21, 2022 09:43
@dumbbell dumbbell marked this pull request as ready for review November 21, 2022 12:15
deps/rabbit/src/rabbit_db.erl Outdated Show resolved Hide resolved
deps/rabbit/src/rabbit_db.erl Outdated Show resolved Hide resolved
@dumbbell dumbbell force-pushed the reorganize-code-for-khepri-phase1 branch from 41627e0 to 2c5d75e Compare November 23, 2022 10:09
@dumbbell dumbbell marked this pull request as draft November 23, 2022 18:03
@dumbbell
Copy link
Member Author

dumbbell commented Nov 23, 2022

I converted this patch back a draft. I'm preparing another patch to sort out the Mnesia directory issue.

I will later update this patch accordingly.

Update: See #6462.

@dumbbell dumbbell force-pushed the reorganize-code-for-khepri-phase1 branch from 2c5d75e to bc523c3 Compare December 1, 2022 10:14
@dumbbell dumbbell marked this pull request as ready for review December 1, 2022 10:15
@PraveenNanda124

This comment was marked as spam.

@dumbbell dumbbell force-pushed the reorganize-code-for-khepri-phase1 branch from bc523c3 to 1f5370e Compare December 9, 2022 09:47
The second argument is a list of tuples, not one tuple.
One testsuite was using strings to check the non-existence of a user and
a virtual host. Given these names are expected to be binaries, for sure
strings won't match.
…time_parameters`

So far, `rabbit_runtime_parameters` could be called with invalid terms.
There are probably still places in the code which lack this kind of
verification.
RabbitMQ lacks argument verifications in many places unfortunately.
Several testcases were passing bad arguments to the command but were
also comparing the result to bad data. This happened to "work"...
@dumbbell dumbbell force-pushed the reorganize-code-for-khepri-phase1 branch from 1f5370e to 527984c Compare December 13, 2022 13:56
The idea is to isolate in those modules the code to handle the records
in the database. For now, we only support Mnesia, but there is work in
progress to move to another database engine. Doing this reorganization
of the code will also isolate the changes of this upcoming database
engine switch, making them easier to rebase and review.

In the end, the `rabbit_db*` modules should export functions for each
operations we want to perform. Things like "add this record", "list
records matching that", "update a record using an anonymous function
inside a transaction", etc.

Only the following subsystems are impacted in this commit:
* virtual vhosts
* internal users
* runtime parameters

They were modified together because they depend on each other.
This patch adapts several testcases to this behavior change. The goal is
to avoid transactions where there is no need for one.
@dumbbell dumbbell force-pushed the reorganize-code-for-khepri-phase1 branch from 527984c to 46b71e8 Compare December 14, 2022 09:07
@dumbbell dumbbell merged commit cf196fe into main Dec 14, 2022
@dumbbell dumbbell deleted the reorganize-code-for-khepri-phase1 branch December 14, 2022 09:28
@dumbbell dumbbell added this to the 3.12.0 milestone Dec 14, 2022
@dumbbell
Copy link
Member Author

This change was intended to be a non-functional change. In the end, it's not entirely the case: deleting a non-existing vhost or user succeeds (the delete operations are idempotent). Before, it would have thrown an exception. This simplifies the transactional code a lot.

dumbbell added a commit that referenced this pull request Dec 14, 2022
…eted

This was the case before the reorganize of the code made in #6430 and
therefore a regression in the mentionned pull request. This patch
restores the behavior.

Note that this is already like that for internal users. Thus, the
behavior and `rabbit_db_*` are consistent.
dumbbell pushed a commit that referenced this pull request Jan 13, 2023
This should have been handled in #6430 but was missed unfortunately.
dumbbell pushed a commit that referenced this pull request Jan 13, 2023
This should have been handled in #6430 but was missed unfortunately.
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.

4 participants