Skip to content

Use OTP 26 features #8553

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

Closed
wants to merge 6 commits into from
Closed

Use OTP 26 features #8553

wants to merge 6 commits into from

Conversation

ansd
Copy link
Member

@ansd ansd commented Jun 13, 2023

See commit messages.
RabbitMQ 3.13 will require OTP 26 as minimum required version.
The OTP 25 GitHub Action tests are expected to fail.

@ansd ansd force-pushed the min-otp-26 branch 3 times, most recently from b8f6d14 to e411da5 Compare June 13, 2023 16:45
@ansd ansd marked this pull request as ready for review June 13, 2023 16:46
@michaelklishin
Copy link
Collaborator

@ansd should we bump the minimum required version then and delete Erlang 25.3 runs from Actions?

@michaelklishin
Copy link
Collaborator

We'd have to also bump the minimum required version in rabbitmq/rabbitmq-packaging (I assume main there is only used by the 3.13 pipeline).

@ansd ansd requested review from lhoguin and dcorbacho June 14, 2023 11:11
@ansd ansd requested a review from lhoguin June 15, 2023 13:26
@ansd
Copy link
Member Author

ansd commented Jun 15, 2023

@pjk25 does the PR contain everything needed to bump the minimum OTP to 26? I'm not familiar with what exactly needs to be updated in the Bazel files. It seems we will still require OTP 25 for mixed version tests because the lower version nodes will continue to be OTP 25?

lhoguin
lhoguin previously approved these changes Jun 15, 2023
@HoloRin
Copy link
Contributor

HoloRin commented Jun 15, 2023

@pjk25 does the PR contain everything needed to bump the minimum OTP to 26? I'm not familiar with what exactly needs to be updated in the Bazel files. It seems we will still require OTP 25 for mixed version tests because the lower version nodes will continue to be OTP 25?

I think you have covered what is necessary. Mixed version tests use the same erlang for all nodes, so the erlang version must be supported by both. Furthermore, we don't currently differentiate between the erlang version used for compilation, vs the version used to run tests. So, for mixed version tests on this branch, as currently configured, compilation of both versions of the broker, and running of tests all happens on 25.3. Supporting something more complicated will require some further enhancements to the build system.

dcorbacho
dcorbacho previously approved these changes Jun 15, 2023
@ansd ansd dismissed stale reviews from dcorbacho and lhoguin via f185a32 June 15, 2023 14:17
@ansd
Copy link
Member Author

ansd commented Jun 15, 2023

Thank you. This means mixed version tests must run on OTP 26. I bumped that version in the latest commit.
The currently configured lower version is 3.11.11 which doesn't support 26.
So, we have to merge #8341 first.

@michaelklishin
Copy link
Collaborator

@ansd we can bump to 3.11.18 which has virtually every Erlang 26-related change that 3.12.0 has.

@HoloRin
Copy link
Contributor

HoloRin commented Jun 16, 2023

@ansd we can bump to 3.11.18 which has virtually every Erlang 26-related change that 3.12.0 has.

#8584

ansd added 5 commits June 22, 2023 08:25
Routing to nowhere with the direct exchange is now a bit faster:
```
java -jar target/perf-test.jar -x 2 -y 0 -k route-nowhere -z 60
```

Remove dead code rabbit_db_binding:route_v2/3
as selective receives are efficient in OTP 26:
```
OTP-18431
Application(s):
compiler, stdlib
Related Id(s):
PR-6739
Improved the selective receive optimization, which can now be enabled for references returned from other functions.
This greatly improves the performance of gen_server:send_request/3, gen_server:wait_response/2, and similar functions.
```
Since Erlang/OTP 26:
```
OTP-18445
Application(s):
erts, stdlib
It is no longer necessary to enable a feature in the runtime system in order to load modules that are using it.
It is sufficient to enable the feature in the compiler when compiling it.
That means that to use feature maybe_expr in Erlang/OTP 26, it is sufficient to enable it during compilation.
In Erlang/OTP 27, feature maybe_expr will be enabled by default, but it will be possible to disable it.
```
and delete Erlang 25.3 runs from Actions
Mixed version tests use the same erlang for all nodes, so the erlang version must be supported by both new and old version.
This test fails when MQTT client ID tracking is performed in Ra, and the
higher version node gets compiled with a different OTP version (26) than
the lower version node (25).

The reason is described in 83eede7
```
An interesting side note learned here is that the compiled file
rabbit_mqtt_collector must not be changed. This commit only modifies
function specs. However as soon as the compiled code is changed, this
module becomes a new version. The new version causes the anonymous ra query
function to fail in mixed clusters: When the old node does a
ra:leader_query where the leader is on the new node, the query function
fails on the new node with `badfun` because the new node does not have
the same module version. For more context, read:
https://web.archive.org/web/20181017104411/http://www.javalimit.com/2010/05/passing-funs-to-other-erlang-nodes.html
```

We shouldn’t use an anonymous function for ra:leader_query or ra:consistent_query.
Instead we should use the {M,F,A} form.
https://github.com/rabbitmq/ra/blob/9e5d437a0a76cc126f396be93645a290e758ac75/src/ra.erl#L102-L103

In MQTT the anonymous function is used in https://github.com/rabbitmq/rabbitmq-server/blob/bcb95c949d8622fb7ba6d3c1b4a12fdb73b0fa66/deps/rabbitmq_mqtt/src/rabbit_mqtt_collector.erl#L50
This causes the query to return a bad fun error (silently ignored in https://github.com/rabbitmq/rabbitmq-server/blob/bcb95c949d8622fb7ba6d3c1b4a12fdb73b0fa66/deps/rabbitmq_mqtt/src/rabbit_mqtt_collector.erl#L70-L71 )
when executed on a different node and either:
1.) Any code in file rabbit_mqtt_collector.erl changed, or
2.) The code gets compiled with a different OTP version.

2.) is the reason for a failing mixed version test in #8553 because both higher and lower versions run OTP 26,
but the higher version node got compiled with 26 while the lower version node got compiled with 25.

The same file
compiled with OTP 26.0.1
```
1> rabbit_mqtt_collector:module_info(attributes).
[{vsn,[30045739264236496640687548892374951597]}]
```

compiled with OTP 25.3.2
```
1> rabbit_mqtt_collector:module_info(attributes).
[{vsn,[168144385419873449889532520247510637232]}]
```

Due to the very low impact that maintenance mode will not close all MQTT
client connections with feature flag delete_ra_cluster_mqtt_node being
disabled, we skip this test.
@ansd
Copy link
Member Author

ansd commented Jun 23, 2023

I revert this PR to a draft as I opened it too early and we ended up mixing multiple issues in this single PR. We should first create a separate PR which only bumps the minimum required version to OTP 26 fixing all tests such as the failing rabbit_stream_queue_SUITE-mixed group cluster_size_2 case recover. Once all these tests are green and that other PR has been merged, we go the next step and actually use OTP 26 features as done in this PR.

@ansd ansd marked this pull request as draft June 23, 2023 08:37
ansd added a commit that referenced this pull request Oct 27, 2023
This test fails when MQTT client ID tracking is performed in Ra, and the
higher version node gets compiled with a different OTP version (26) than
the lower version node (25).

The reason is described in 83eede7
```
An interesting side note learned here is that the compiled file
rabbit_mqtt_collector must not be changed. This commit only modifies
function specs. However as soon as the compiled code is changed, this
module becomes a new version. The new version causes the anonymous ra query
function to fail in mixed clusters: When the old node does a
ra:leader_query where the leader is on the new node, the query function
fails on the new node with `badfun` because the new node does not have
the same module version. For more context, read:
https://web.archive.org/web/20181017104411/http://www.javalimit.com/2010/05/passing-funs-to-other-erlang-nodes.html
```

We shouldn’t use an anonymous function for ra:leader_query or ra:consistent_query.
Instead we should use the {M,F,A} form.
https://github.com/rabbitmq/ra/blob/9e5d437a0a76cc126f396be93645a290e758ac75/src/ra.erl#L102-L103

In MQTT the anonymous function is used in https://github.com/rabbitmq/rabbitmq-server/blob/bcb95c949d8622fb7ba6d3c1b4a12fdb73b0fa66/deps/rabbitmq_mqtt/src/rabbit_mqtt_collector.erl#L50
This causes the query to return a bad fun error (silently ignored in https://github.com/rabbitmq/rabbitmq-server/blob/bcb95c949d8622fb7ba6d3c1b4a12fdb73b0fa66/deps/rabbitmq_mqtt/src/rabbit_mqtt_collector.erl#L70-L71 )
when executed on a different node and either:
1.) Any code in file rabbit_mqtt_collector.erl changed, or
2.) The code gets compiled with a different OTP version.

2.) is the reason for a failing mixed version test in #8553 because both higher and lower versions run OTP 26,
but the higher version node got compiled with 26 while the lower version node got compiled with 25.

The same file
compiled with OTP 26.0.1
```
1> rabbit_mqtt_collector:module_info(attributes).
[{vsn,[30045739264236496640687548892374951597]}]
```

compiled with OTP 25.3.2
```
1> rabbit_mqtt_collector:module_info(attributes).
[{vsn,[168144385419873449889532520247510637232]}]
```

Due to the very low impact that maintenance mode will not close all MQTT
client connections with feature flag delete_ra_cluster_mqtt_node being
disabled, we skip this test.
@ansd
Copy link
Member Author

ansd commented Oct 27, 2023

Closing since this PR has been delivered by the following individual PRs:
#9787
#9805
#9808
#9809
#9810

@ansd ansd closed this Oct 27, 2023
@ansd ansd deleted the min-otp-26 branch October 27, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants