Print warning when incompatible RabbitMQ version is used #5315
Conversation
|
This still needs an update on the documentation, but I am not sure what we prefer:
If we want to provide instructions on installing an older version on various platforms, or detailed instructions to change the configuration (if possible), then this will become quite complex. Finding a good place in the docs such that it doesn't interrupt the flow but is still findable will be tricky. It will also be more difficult to keep up to date. The wiki would be more flexible in that regard. |
|
Hmm, conda build still failing claiming |
There was a problem hiding this comment.
I'm leaving a couple of comments.
I would also suggest to change verdi status so it shows an error (instead of an OK) if the version is incompatible, with some short but clear message "Incompatible RabbitMQ version detected, see https://..."
Apart from this, I'm OK.
I suggest to keep the information on the wiki (it's indeed something we want to keep improving, even after release; and we hope to eventually fix this and remove this wiki page), but I suggest instead to add (already in this PR? or anyway before closing #5300) some links to that page in the documentation for the installation of AiiDA.
| version = parse(communicator.server_properties['version'].decode('utf-8')) | ||
| if version >= parse('3.8'): | ||
| echo.echo_warning(f'Connected to RabbitMQ v{version} which is known to cause problems)') | ||
| echo.echo_warning(f'See {url} for details.') |
There was a problem hiding this comment.
Is it OK to use the cmdline module functions like echo outside of it? Maybe this function should just return True/Fals, and the warning be issued elsewhere.
There was a problem hiding this comment.
Here it is I feel. The echo commands essentially just directly use the logging module and when invoked through verdi it will simply be configured to write to stdout. When I refactored it to be that way, I decided to just not remove it from the aiida.cmdline package since I know other packages were using it as well and so it would break. That being said, it probably should not be used everywhere else internally.
There was a problem hiding this comment.
Ok, I see. I think it would still be good to avoid directly importing from there - maybe move the function in some utils module, and keep the aiida.cmdline for backward compatibility, proxying to the utils function? Anyway, just a suggestion, not directly related to this issue
There was a problem hiding this comment.
I think it should actually stay in the aiida.cmdline module, because that is what it was designed for. So far it is only used in migrations in aiida.backend and in the aiida.manage module, which I think is acceptable. However, if we want to improve this, we should probably just look into having these modules use the normal logging.warning and equivalent and then make sure that their formatters are set to work just as the one for the command line logger. This way the log messages will be formatted uniformly. Will open an issue for this.
The lower limit for the `kiwipy` requirement is upped to `>=0.7.5` since that provides the `server_properties` property on the `RmqCommunicator`. This allows us to determine the version of the RabbitMQ server that is configured for the profile. In `load_profile` a check is added that will print a warning if this version is greater than 3.7. For RabbitMQ 3.8 and up, the default configuration will have a default timeout on channels of around 30 minutes. This means that tasks that are not acknowledged within that timelimit will be requeued. If the task was actually still running with a daemon worker, it can be started by a second runner simultaneously with all sorts of problems as a consequence.
sphuber
force-pushed
the
fix/5300/warn-rabbitmq-version
branch
from
9113866 to
4967108
Compare
|
Thanks for the review @giovannipizzi . Addressed the comments |
sphuber
force-pushed
the
fix/5300/warn-rabbitmq-version
branch
from
0168d2c to
fd808b3
Compare
sphuber
force-pushed
the
fix/5300/warn-rabbitmq-version
branch
from
fd808b3 to
c0c6e35
Compare
There was a problem hiding this comment.
I'm approving it, however two notes before merging:
- check my comment above on using
aiida.cmdline, if you don't want to solve this now, maybe it's for an issue (to check all usages of cmdline outside of it and replace with utils functions) - if you merge this, don't close Warn early and clearly user when using incompatible RabbitMQ version #5300, or close it but add another issue (still in the 2.0 milestone) to update the docs pointing to the wiki page
Codecov Report
@@ Coverage Diff @@
## develop #5315 +/- ##
===========================================
+ Coverage 82.02% 82.03% +0.02%
===========================================
Files 533 533
Lines 38257 38277 +20
===========================================
+ Hits 31376 31397 +21
+ Misses 6881 6880 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
) The lower limit for the `kiwipy` requirement is upped to `>=0.7.5` since that provides the `server_properties` property on the `RmqCommunicator`. This allows us to determine the version of the RabbitMQ server that is configured for the profile. In `load_profile` a check is added that will print a warning if this version is greater than 3.7. For RabbitMQ 3.8.15 and up, the default configuration will have a default timeout on channels of around 30 minutes. This means that tasks that are not acknowledged within that timelimit will be requeued. If the task was actually still running with a daemon worker, it can be started by a second runner simultaneously with all sorts of problems as a consequence. The breaking change was officially only added in 3.9: rabbitmq/rabbitmq-server@80e3992 but it was backported in the v3.8 minor version in v3.8.15. Cherry-pick: ad28e9b
The lower limit for the `kiwipy` requirement is upped to `>=0.7.5` since
that provides the `server_properties` property on the `RmqCommunicator`.
This allows us to determine the version of the RabbitMQ server that is
configured for the profile. In `load_profile` a check is added that will
print a warning if this version is greater than 3.7.
For RabbitMQ 3.8.15 and up, the default configuration will have a default
timeout on channels of around 30 minutes. This means that tasks that are
not acknowledged within that timelimit will be requeued. If the task was
actually still running with a daemon worker, it can be started by a
second runner simultaneously with all sorts of problems as a consequence.
The breaking change was officially only added in 3.9:
rabbitmq/rabbitmq-server@80e3992
but it was backported in the v3.8 minor version in v3.8.15.
Cherry-pick: ad28e9b
Fixes #5300
The lower limit for the
kiwipyrequirement is upped to>=0.7.5sincethat provides the
server_propertiesproperty on theRmqCommunicator.This allows us to determine the version of the RabbitMQ server that is
configured for the profile. In
load_profilea check is added that willprint a warning if this version is greater than 3.7.
For RabbitMQ 3.8 and up, the default configuration will have a default
timeout on channels of around 30 minutes. This means that tasks that are
not acknowledged within that timelimit will be requeued. If the task was
actually still running with a daemon worker, it can be started by a
second runner simultaneously with all sorts of problems as a consequence.