Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send ocr processing messages to active mq #60

Merged
merged 9 commits into from
Jun 8, 2023

Conversation

markusweigelt
Copy link
Collaborator

@markusweigelt markusweigelt commented May 16, 2023

  • add functions to send task action messages to ocrd_lib.sh
  • update activemq client
  • use process function to set task from OPEN state to INWORK
  • use close function to close task over task action message queue

comments_popup_with_ocrd_messages

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

@markusweigelt markusweigelt requested a review from bertsky May 16, 2023 16:02
Copy link
Member

@bertsky bertsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
  2. in case the Kitodo side does not have the right version installed, shouldn't we fall back to the old KitodoProduction.FinalizeStep.Queue interface?
  3. 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

@markusweigelt
Copy link
Collaborator Author

markusweigelt commented May 22, 2023

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
Copy link
Member

bertsky commented May 23, 2023

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
Copy link
Collaborator Author

markusweigelt commented Jun 7, 2023

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.

@markusweigelt markusweigelt requested a review from bertsky June 7, 2023 15:16
ocrd_lib.sh Show resolved Hide resolved
ocrd_lib.sh Outdated Show resolved Hide resolved
process_images.sh Outdated Show resolved Hide resolved
@bertsky
Copy link
Member

bertsky commented Jun 7, 2023

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...)

markusweigelt and others added 3 commits June 7, 2023 18:00
Copy link
Member

@bertsky bertsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

@markusweigelt markusweigelt merged commit d4a1229 into main Jun 8, 2023
@markusweigelt markusweigelt deleted the add-task-action-messages branch June 8, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants