Bug 27661 - Clarify error for message broker
Summary: Clarify error for message broker
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: About (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 22417
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-09 13:28 UTC by Martin Renvoize
Modified: 2021-03-05 16:45 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04


Attachments
Bug 27661: Update about for message broker status (3.26 KB, patch)
2021-02-09 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27661: Update about for message broker status (3.33 KB, patch)
2021-02-12 20:22 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27661: Update about for message broker status (3.38 KB, patch)
2021-02-14 14:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27661: (QA follow-up) Capitalization (1.04 KB, patch)
2021-02-14 14:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27661: (QA follow-up) Use bootstrap classes (8.16 KB, patch)
2021-02-18 17:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27661: (QA follow-up) Use bootstrap classes (8.23 KB, patch)
2021-02-18 18:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27661: Use td.bg-success for ES and memcached as well (2.14 KB, patch)
2021-02-19 07:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27661: Update about for message broker status (3.48 KB, patch)
2021-02-19 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27661: (QA follow-up) Capitalization (1.13 KB, patch)
2021-02-19 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27661: (QA follow-up) Use bootstrap classes (8.33 KB, patch)
2021-02-19 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27661: Use td.bg-success for ES and memcached as well (2.23 KB, patch)
2021-02-19 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-02-09 13:28:49 UTC
'Connection failed' is a bit strong for the case where RabbitMQ is not found.

In reality, we fallback to polling the database so Koha will function reasonably without the overhead of RabbitMQ.  We should clarify that on the about page.
Comment 1 Martin Renvoize 2021-02-09 13:56:38 UTC
Created attachment 116575 [details] [review]
Bug 27661: Update about for message broker status

This patch updates the about page with regards to the message broker
status and adds some small formatting improvements for displaying the
status.

Test plan
1/ Run through the yarn build process to update the CSS file.
2/ View the about page without RabbitMQ installed
3/ View the about page with RabbitMQ installed
4/ Note the formatting and message change and signoff
Comment 2 Owen Leonard 2021-02-12 20:22:39 UTC
Created attachment 116848 [details] [review]
Bug 27661: Update about for message broker status

This patch updates the about page with regards to the message broker
status and adds some small formatting improvements for displaying the
status.

Test plan
1/ Run through the yarn build process to update the CSS file.
2/ View the about page without RabbitMQ installed
3/ View the about page with RabbitMQ installed
4/ Note the formatting and message change and signoff

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Katrin Fischer 2021-02-14 14:15:13 UTC
Created attachment 116855 [details] [review]
Bug 27661: Update about for message broker status

This patch updates the about page with regards to the message broker
status and adds some small formatting improvements for displaying the
status.

Test plan
1/ Run through the yarn build process to update the CSS file.
2/ View the about page without RabbitMQ installed
3/ View the about page with RabbitMQ installed
4/ Note the formatting and message change and signoff

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Katrin Fischer 2021-02-14 14:15:17 UTC
Created attachment 116856 [details] [review]
Bug 27661: (QA follow-up) Capitalization

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 5 Jonathan Druart 2021-02-15 09:54:53 UTC
+.status_fatal {
+    background-color: #FF6347 !important;

Why tomato? :)

We already have 
.error {
    color: #CC0000;
}
Comment 6 Jonathan Druart 2021-02-15 11:26:31 UTC
This change is actually not advertised by the patch and not directly linked with what we are doing here. Can we remove it from the patch?
Comment 7 Martin Renvoize 2021-02-18 16:01:16 UTC
(In reply to Jonathan Druart from comment #5)
> +.status_fatal {
> +    background-color: #FF6347 !important;
> 
> Why tomato? :)
> 
> We already have 
> .error {
>     color: #CC0000;
> }

Fair point, I had forgotten we had 'error' already.. kinda annoying this table has it's own classing in the first place to be honest.

Would you prefer a css change to fix the color to match our .error class.. or wider template followup to convert to using the existing classes..

I'm thinking the latter.
Comment 8 Martin Renvoize 2021-02-18 16:12:34 UTC
Hmm, I find the stark red as a background a bit challenging to read.. that's probably why I went for Tomato..  Must admit, I find the inconsistency in the about page upsets my OCD.. some status's are on their own line, others are inline.. some have background colors applied to a span, whilst others still have the whole row highlighted.
Comment 9 Martin Renvoize 2021-02-18 17:00:19 UTC
Created attachment 117006 [details] [review]
Bug 27661: (QA follow-up) Use bootstrap classes

This patch removes the local status_* css rules in favour of using the
bg-* rules that are shipped with the bootstrap version we are using and
then updates the relevant lines in the template to use those classes.

We also update the default table styling which adds background color to
the odd table rows so that our local classes take precidence.
Comment 10 Martin Renvoize 2021-02-18 17:02:00 UTC
Setting back to signed off to attract a second QA run here.. I think the final followup gives us a much cleaner approach to the styling which will allow for more global use of the bootstrap bg-* options for tables.
Comment 11 Martin Renvoize 2021-02-18 17:05:45 UTC
https://getbootstrap.com/docs/3.4/css/#helper-classes-backgrounds for reference of the bootstrap classes I'm talking about
Comment 12 Owen Leonard 2021-02-18 18:00:55 UTC
Created attachment 117010 [details] [review]
Bug 27661: (QA follow-up) Use bootstrap classes

This patch removes the local status_* css rules in favour of using the
bg-* rules that are shipped with the bootstrap version we are using and
then updates the relevant lines in the template to use those classes.

We also update the default table styling which adds background color to
the odd table rows so that our local classes take precidence.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Marcel de Rooy 2021-02-19 07:26:30 UTC
We are advertizing here that we support SQL polling and Rabbit MQ.
Is that really the way we want to go? Support both ways for any new case where we would like to use RabbitMQ?
The SQL polling has been added to the initial use of RabbitMQ as a fallback and probably for building some confidence.

Since I doubt that we will be supporting both ways, I would propose rewording.
Any thoughts?
Comment 14 Jonathan Druart 2021-02-19 07:43:37 UTC
(In reply to Marcel de Rooy from comment #13)
> We are advertizing here that we support SQL polling and Rabbit MQ.
> Is that really the way we want to go? Support both ways for any new case
> where we would like to use RabbitMQ?
> The SQL polling has been added to the initial use of RabbitMQ as a fallback
> and probably for building some confidence.
> 
> Since I doubt that we will be supporting both ways, I would propose
> rewording.
> Any thoughts?

See misc/background_jobs_worker.pl lines 58-63, there is no additional code, and it applies to all jobs. It shouldn't hurt to have it and keep it.
Comment 15 Marcel de Rooy 2021-02-19 07:49:20 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Marcel de Rooy from comment #13)
> > We are advertizing here that we support SQL polling and Rabbit MQ.
> > Is that really the way we want to go? Support both ways for any new case
> > where we would like to use RabbitMQ?
> > The SQL polling has been added to the initial use of RabbitMQ as a fallback
> > and probably for building some confidence.
> > 
> > Since I doubt that we will be supporting both ways, I would propose
> > rewording.
> > Any thoughts?
> 
> See misc/background_jobs_worker.pl lines 58-63, there is no additional code,
> and it applies to all jobs. It shouldn't hurt to have it and keep it.

Thx for clarifying. Had the impression that it was written for a specific job. Which is not the case. Revisiting.
Comment 16 Jonathan Druart 2021-02-19 07:57:41 UTC
Created attachment 117022 [details] [review]
Bug 27661: Use td.bg-success for ES and memcached as well
Comment 17 Marcel de Rooy 2021-02-19 07:59:32 UTC
Created attachment 117023 [details] [review]
Bug 27661: Update about for message broker status

This patch updates the about page with regards to the message broker
status and adds some small formatting improvements for displaying the
status.

Test plan
1/ Run through the yarn build process to update the CSS file.
2/ View the about page without RabbitMQ installed
3/ View the about page with RabbitMQ installed
4/ Note the formatting and message change and signoff

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2021-02-19 07:59:35 UTC
Created attachment 117024 [details] [review]
Bug 27661: (QA follow-up) Capitalization

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2021-02-19 07:59:38 UTC
Created attachment 117025 [details] [review]
Bug 27661: (QA follow-up) Use bootstrap classes

This patch removes the local status_* css rules in favour of using the
bg-* rules that are shipped with the bootstrap version we are using and
then updates the relevant lines in the template to use those classes.

We also update the default table styling which adds background color to
the odd table rows so that our local classes take precidence.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2021-02-19 08:02:02 UTC
Created attachment 117026 [details] [review]
Bug 27661: Use td.bg-success for ES and memcached as well

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Martin Renvoize 2021-02-19 08:23:00 UTC
(In reply to Marcel de Rooy from comment #13)
> We are advertizing here that we support SQL polling and Rabbit MQ.
> Is that really the way we want to go? Support both ways for any new case
> where we would like to use RabbitMQ?
> The SQL polling has been added to the initial use of RabbitMQ as a fallback
> and probably for building some confidence.
> 
> Since I doubt that we will be supporting both ways, I would propose
> rewording.
> Any thoughts?

I think it would be trivial to drop update the change here at the same time as deprecating the polling fallback should we do that in the future.

I'm actually wondering if things might go the other way... I was wary of adding yet another system to run and maintain when we added RabbitMQ personally.. I'm really not sure we need all that it offers and personally would have preferred an approach where we started with an SQL based queue and migrated to a bigger broker only as and when we found we needed it/found that polling was a performance issue.

I'd also like to drop the 'required' state of the STOMP dependency as we don't actually require it to function.. we just require it at the moment as we use it to work out if RabbitMQ is present in the worker daemon.
Comment 22 Martin Renvoize 2021-02-19 08:23:57 UTC
Nice addition also using the classes for ES and Memcached :)
Comment 23 Jonathan Druart 2021-02-23 12:22:07 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 24 Fridolin Somers 2021-02-25 09:46:26 UTC
Pushed to 20.11.x for 20.11.04
Comment 25 Andrew Fuerste-Henry 2021-03-05 16:45:58 UTC
Missing dependency, not backported to 20.05