We added the JobsNotificationMethod preference so people can disable the use of RabbitMQ explicitly and not get the logs flooded with connection errors. But a patch on bug 34070 reintroduced the warn ♪┏(・o・)┛♪ e2e587273c14f092d71046f21266473445b219c5
Created attachment 186506 [details] [review] Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling' The background job workers are showing STOMP/RabbitMQ connection error warnings even when the JobsNotificationMethod system preference was set to 'polling', which doesn't require a message broker connection. This patch adds checks for the JobsNotificationMethod preference before displaying connection warnings, so warnings only appear when STOMP is actually expected to be available. Changes: - misc/workers/background_jobs_worker.pl: Check syspref before warning - misc/workers/es_indexer_daemon.pl: Check syspref before warning Test plan: 1. Set JobsNotificationMethod to 'polling' 2. Run background_jobs_worker.pl - should not show STOMP warnings 3. Run es_indexer_daemon.pl - should not show STOMP warnings 4. Set JobsNotificationMethod to 'STOMP' without RabbitMQ running 5. Run workers - should show appropriate STOMP connection warnings 6. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 186507 [details] [review] Bug 40820: (follow-up) es_indexer_daemon.pl missed on 34070 The es_indexer_daemon.pl was missing handling for the case where Koha::BackgroundJob->connect() returns undef without throwing an exception (which happens when connection fails silently). This adds the missing warning for that case, matching the behavior in background_jobs_worker.pl. Test plan: 1. Set JobsNotificationMethod to 'STOMP' 2. Configure invalid STOMP settings that cause connect() to return undef 3. Run es_indexer_daemon.pl - should show connection warning 4. Verify jobs are still processed via database polling Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi (tcohen) from comment #0) > We added the JobsNotificationMethod preference so people can disable the use > of RabbitMQ explicitly and not get the logs flooded with connection errors. > > But a patch on bug 34070 reintroduced the warn ♪┏(・o・)┛♪ It took me a few minutes to wrap my head around this one but I see it now. Bug 34070 added the following: '$error ||= "Cannot connect to the message broker, the jobs will be processed anyway" unless $conn;' I didn't even know about bug 35655 until this moment. > e2e587273c14f092d71046f21266473445b219c5 I don't get anything for that commit hash? -- At a glance I think these patches should make sense...
(In reply to David Cook from comment #3) > (In reply to Tomás Cohen Arazi (tcohen) from comment #0) > > We added the JobsNotificationMethod preference so people can disable the use > > of RabbitMQ explicitly and not get the logs flooded with connection errors. > > > > But a patch on bug 34070 reintroduced the warn ♪┏(・o・)┛♪ > > It took me a few minutes to wrap my head around this one but I see it now. > Bug 34070 added the following: > '$error ||= "Cannot connect to the message broker, the jobs will be > processed anyway" unless $conn;' > > I didn't even know about bug 35655 until this moment. > > > e2e587273c14f092d71046f21266473445b219c5 > > I don't get anything for that commit hash? In a hurry, I just grabbed the hash from the submitted patch on bug 34070 https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182574 but you guessed right. I believe 34070 patches were wrong because they treated the 'skip if polling' behavior as an error by not acknowledging bug 35655. The code has been there for a good while and this caused a regression. > At a glance I think these patches should make sense... The patches are trivial and just wrap things around a check on JobsNotificationMethod='STOMP' which is also correct. But if we don't call ->connect() unless STOMP, then we don't need the code handling non-STOMP in ->connect. So any of the refactoring efforts should clear this for good I'd say. I might try to resurrect those patches...
We need to improve the logging here. We actually do not log the $error from the workers because ->connect does not rethrow the exception. Cannot connect to broker (Failed to connect: Error connecting to localhost:61613: Connection refused at /usr/share/perl5/Net/Stomp.pm line 27.; giving up at /usr/share/perl5/Net/Stomp.pm line 27.
(In reply to Jonathan Druart from comment #5) > We need to improve the logging here. > > We actually do not log the $error from the workers because ->connect does > not rethrow the exception. > > Cannot connect to broker (Failed to connect: Error connecting to > localhost:61613: Connection refused at /usr/share/perl5/Net/Stomp.pm line > 27.; giving up at /usr/share/perl5/Net/Stomp.pm line 27. Sorry, forget that. It's the point of "Bug 34070: Display the 'jobs will be processed anyway' message from worker" I was missing the worker logs in my tail -f
I don't understand the discussion. > I might try to resurrect those patches... The patches look good to me and I am ready to signoff. What's missing?
(In reply to Jonathan Druart from comment #7) > I don't understand the discussion. > > > I might try to resurrect those patches... > > The patches look good to me and I am ready to signoff. What's missing? On this bug, there's nothing missing.
Created attachment 186620 [details] [review] Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling' The background job workers are showing STOMP/RabbitMQ connection error warnings even when the JobsNotificationMethod system preference was set to 'polling', which doesn't require a message broker connection. This patch adds checks for the JobsNotificationMethod preference before displaying connection warnings, so warnings only appear when STOMP is actually expected to be available. Changes: - misc/workers/background_jobs_worker.pl: Check syspref before warning - misc/workers/es_indexer_daemon.pl: Check syspref before warning Test plan: 1. Set JobsNotificationMethod to 'polling' 2. Run background_jobs_worker.pl - should not show STOMP warnings 3. Run es_indexer_daemon.pl - should not show STOMP warnings 4. Set JobsNotificationMethod to 'STOMP' without RabbitMQ running 5. Run workers - should show appropriate STOMP connection warnings 6. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 186621 [details] [review] Bug 40820: (follow-up) es_indexer_daemon.pl missed on 34070 The es_indexer_daemon.pl was missing handling for the case where Koha::BackgroundJob->connect() returns undef without throwing an exception (which happens when connection fails silently). This adds the missing warning for that case, matching the behavior in background_jobs_worker.pl. Test plan: 1. Set JobsNotificationMethod to 'STOMP' 2. Configure invalid STOMP settings that cause connect() to return undef 3. Run es_indexer_daemon.pl - should show connection warning 4. Verify jobs are still processed via database polling Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 186816 [details] [review] Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling' The background job workers are showing STOMP/RabbitMQ connection error warnings even when the JobsNotificationMethod system preference was set to 'polling', which doesn't require a message broker connection. This patch adds checks for the JobsNotificationMethod preference before displaying connection warnings, so warnings only appear when STOMP is actually expected to be available. Changes: - misc/workers/background_jobs_worker.pl: Check syspref before warning - misc/workers/es_indexer_daemon.pl: Check syspref before warning Test plan: 1. Set JobsNotificationMethod to 'polling' 2. Run background_jobs_worker.pl - should not show STOMP warnings 3. Run es_indexer_daemon.pl - should not show STOMP warnings 4. Set JobsNotificationMethod to 'STOMP' without RabbitMQ running 5. Run workers - should show appropriate STOMP connection warnings 6. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 186817 [details] [review] Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling' The background job workers are showing STOMP/RabbitMQ connection error warnings even when the JobsNotificationMethod system preference was set to 'polling', which doesn't require a message broker connection. This patch adds checks for the JobsNotificationMethod preference before displaying connection warnings, so warnings only appear when STOMP is actually expected to be available. Changes: - misc/workers/background_jobs_worker.pl: Check syspref before warning - misc/workers/es_indexer_daemon.pl: Check syspref before warning Test plan: 1. Set JobsNotificationMethod to 'polling' 2. Run background_jobs_worker.pl - should not show STOMP warnings 3. Run es_indexer_daemon.pl - should not show STOMP warnings 4. Set JobsNotificationMethod to 'STOMP' without RabbitMQ running 5. Run workers - should show appropriate STOMP connection warnings 6. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 186818 [details] [review] Bug 40820: (follow-up) es_indexer_daemon.pl missed on 34070 The es_indexer_daemon.pl was missing handling for the case where Koha::BackgroundJob->connect() returns undef without throwing an exception (which happens when connection fails silently). This adds the missing warning for that case, matching the behavior in background_jobs_worker.pl. Test plan: 1. Set JobsNotificationMethod to 'STOMP' 2. Configure invalid STOMP settings that cause connect() to return undef 3. Run es_indexer_daemon.pl - should show connection warning 4. Verify jobs are still processed via database polling Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work everyone! Pushed to main for 25.11
Nice work everyone! Pushed to 25.05.x
Pushed into 24.11.x for 24.11.11 nice work everyone