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

Webapi spec update #250

Merged
merged 23 commits into from
Jan 25, 2024
Merged

Webapi spec update #250

merged 23 commits into from
Jan 25, 2024

Conversation

joschrew
Copy link
Contributor

I think this PR is currently nothing more than a draft and a call for discussion. At least for me some things are not clear and I cannot just update it without further information. This PR contains some commits all for the same document. With this I tried to make more clear what I wanted to change and what was intended.

Here are my open questions:
General questions:

  • What is our goal with this document? Probably we should discuss this and adapt it accordingly because currently it is not clear to me and I don't know what is missing or what is too much. Previously to me it seemed to be a mixture of a description of the webAPI-API-Description document, an architecture proposal and a description of the ocrd-network-package we have implemented.
  • Do we want to describe the implementation in core? Do we want to describe the workflow endpoint we are currently developing?

More detailed question regarding the current texts:

  • I am not sure about the section ### Process Queue. I don't think redeliver is used. And don't think messages are re-enqueued. They should not fail, otherwise it is a bug (not sure) in the code. Processors can fail though, but then the message should not be processed again
  • What about the result queue? I think we have still parts of it in the code, but I think we don't really use it and the result_callback should be sufficient. Maybe we could remove the text belonging to the result queue and clean up the code in core at one point in time.

Questions more related to the webAPI in general:

  • Our workflow endpoint currently does not fit to the webAPI. In core we could think about changing that or changing the webAPI to fit to the endpoint.
  • We could implement the remainder of the workflow server in core. If we have the workflow endpoint, managing workflows is not that hard. We could use the mongodb to store workflows and circumvent the storage problem.

@joschrew joschrew requested a review from tdoan2010 September 28, 2023 07:45
@tdoan2010
Copy link
Contributor

tdoan2010 commented Oct 4, 2023

What is our goal with this document?

It should contain a detailed description about the Web API, e.g., how it was implemented, how should it be used. Don't worry that it's too much.

Do we want to describe the implementation in core?

Yes. We can describe it in word, or better, with activity diagrams.

Do we want to describe the workflow endpoint we are currently developing?

Yes.

And don't think messages are re-enqueued.

Why not? If somehow a problem occurs during the processing, the message should be re-queued so that the Worker can try processing it again. If ACK is not sent, or a NACK is set, then the message will be re-queued.

What about the result queue?

Just leave it there. It's already implemented. People can decide which approach they want to use, result queue or callback.

Our workflow endpoint currently does not fit to the webAPI. In core we could think about changing that or changing the webAPI to fit to the endpoint.

Yes, the openapi.yml file is completely outdated. Please update it to the current implementation in the code.

We could implement the remainder of the workflow server in core.

Theoretically yes, but let's discuss that later.

@joschrew
Copy link
Contributor Author

Thanks for the answer. I have added the activity diagrams for worker and processing-endpoint.

Regarding re-enqueue: the code regarding this is channel.basic_nack(delivery_tag=delivery_tag, multiple=False, requeue=False). The messages are currently not re-enqueued due to requeue=False but I will ask about that in our next Monday-meeting before changing the spec.

Ids for jobs of processing and workflows are unique across all jobs, so
path to get information about this jobs can be simplified
Still things left to be discussed and changed
@joschrew
Copy link
Contributor Author

I have updated the activity diagram for the Processing Server and the webapi-openapi-spec. To the webapi I rather added unfinished work (TODO comments), because there are some questions left which I wasn't able to answer on my own. So this needs further discussions in one of our meetings on Monday probably.

joschrew and others added 4 commits November 21, 2023 14:57
@tdoan2010
Copy link
Contributor

@joschrew please add the command to start a Processor Server under section 6.5.

@joschrew
Copy link
Contributor Author

joschrew commented Dec 5, 2023

I updated the path in the web_api.md to the recent changes to the openapi.yml and I changed some text in the Terminology section related to these changes (only text changes in that 6 lines).
Additionally the Images web-api-distributed.jpg and web-api-distributed-queue.jpg have a path inside which does not fit any more. It says POST /processor/{executable} but should bePOST /processor/run/{executable}. @tdoan2010, please update this two images if possible.
I have read through the web_api.md and I think it is good now and I would change this PR to "ready to be merged" if you think so too ...

@tdoan2010
Copy link
Contributor

tdoan2010 commented Dec 5, 2023

I've updated the path as you asked.

@tdoan2010 tdoan2010 requested a review from kba December 5, 2023 15:34
@tdoan2010 tdoan2010 marked this pull request as ready for review December 5, 2023 15:35
@kba kba merged commit 95413bf into master Jan 25, 2024
@kba kba deleted the webapi-spec-update branch January 25, 2024 12:14
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.

3 participants