khepri_machine: Request delete_reason if effective machine version >= 2 only

[dumbbell]

Why

The delete_reason property to return was introduced with machine version 2. Older versions have two issues:

• They don't know this property
• They crash on unknown properties

How

We request this new property only if the effective machine version is greater or equal to 2 to avoid the issues listed above.

Future unknown properties are ignored since commit 257cf32 merged in the previous pull request #308.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.83%. Comparing base (0db160f) to head (e3d959c).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #309      +/-   ##
==========================================
+ Coverage   89.58%   89.83%   +0.25%     
==========================================
  Files          22       22              
  Lines        3263     3277      +14     
==========================================
+ Hits         2923     2944      +21     
+ Misses        340      333       -7     

Flag	Coverage Δ
erlang-25	89.07% <100.00%> (+0.41%)	⬆️
erlang-26	89.68% <100.00%> (+0.44%)	⬆️
erlang-27	89.47% <100.00%> (+0.10%)	⬆️
os-ubuntu-latest	89.80% <100.00%> (+0.31%)	⬆️
os-windows-latest	89.65% <100.00%> (+0.32%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dumbbell]

@the-mikedavis: As briefly stated in the commit message and a comment in the code, getting the effective machine version when handling transactions is a bit tricky. The code runs inside the Ra server process. It doesn’t have the store ID because we don’t keep it in the machine state, even though we get it in the init arguments…

For now, I just query the registered name of the process, but, yeah, not ideal… Ideally, I would modify the state to add the store ID. We already bump the machine version anyway. I was lazy for this first iteration :-)

So, I will prepare a new patch to modify that state, and I also need to document that the delete_reason was added as part of version 2.

Regardless, what do you think of the patch so far?

[dumbbell]

I’m wrong: we have the store ID in the machine configuration!

[the-mikedavis]

I found a few typos but otherwise I think this looks great!

[dumbbell]

@the-mikedavis: Did you check if mixed-version testing in RabbitMQ was fixed with this change?

[the-mikedavis]

I didn't test this patch in the server but I was testing ba1bf14 before which should have roughly the same effect. We still need to handle the case of "paused" cluster members on the older version but that change did remove errors from delete_reason not being recognized on older members. I'll update rabbitmq/rabbitmq-server#12753 to test this branch instead

[the-mikedavis]

I updated the draft PR in the server repo to use this change (and #312) and I don't see any error logs about delete_reason in the failed test logs 👍

[dumbbell]

I updated the draft PR in the server repo to use this change (and #312) and I don't see any error logs about delete_reason in the failed test logs 👍

Thank you! I couldn’t reproduce any errors related to this locally either.