slub / ocrd_manager

frontend for ocrd_controller and adapter towards ocrd_kitodo
MIT License
11 stars 3 forks source link

Send ocr processing messages to active mq #60

Closed markusweigelt closed 1 year ago

markusweigelt commented 1 year ago

comments_popup_with_ocrd_messages

(Depends on this Kitodo.Production extension for task action messages.)

markusweigelt commented 1 year ago

Very nice!

I see a couple of points that still need to be addressed:

  1. the ERR trap currently only logs locally, but should now also be used to signal via MQ; I suggest adding a line to logerr which uses kitodo_production_task_action_error_open with a suitable custom message (involving $BASH_COMMAND and/or $(caller) to be as concrete as possible)

For Kitodo I suggest to prevent detailed messages. I think the user should only be informed that a problem occurs and get more information in the monitor to fix the problem.

  1. in case the Kitodo side does not have the right version installed, shouldn't we fall back to the old KitodoProduction.FinalizeStep.Queue interface?

Currently, I am not sure how we can find out. Do we provide another enviroment variable with which to pass the version of Kitodo? Or do we expect the Kitodo version as a parameter to the Kitodo script. Alternatively maybe a flag but we can not introduce flags for every case.

  1. we must make sure that if no Kitodo is configured, then also ACTIVEMQ stays empty. Currently it will read : and fail. So either catch that specifically, or avoid just setting $MQ_HOST:$MQ_PORT in docker-compose.yml

Good point i will change this.

bertsky commented 1 year ago

For Kitodo I suggest to prevent detailed messages. I think the user should only be informed that a problem occurs and get more information in the monitor to fix the problem.

Yes, the details are already in the OCRD logs that can be viewed in the Monitor. We could also just use your default message – but we must add the signal.

Do we provide another enviroment variable with which to pass the version of Kitodo? Or do we expect the Kitodo version as a parameter to the Kitodo script. Alternatively maybe a flag but we can not introduce flags for every case.

IIRC there are a couple of variables available from Kitodo scripts – if the version is among them, that would be the cleanest solution. (We already have an ENV ACTIVEMQ_CLIENT – we could add a runtime variable ACTIVEMQ_PROTOCOL for example.)

Good point i will change this.

In the simplest case, just add another test "$ACTIVEMQ" != :

markusweigelt commented 1 year ago

Changed the implemenation with your hints from review. Now you can configure the queue over env variable and if no variable is set the default queue is used and only kitodo_production_task_action_close does something.

bertsky commented 1 year ago

bertsky approved these changes now

strange that GH sees it this way, although it knows that that review only applied to the last commit (view changes since...)