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

Revise timeout settings #42

Merged
merged 4 commits into from
Mar 8, 2022

Conversation

jojoob
Copy link
Contributor

@jojoob jojoob commented Mar 4, 2022

The changes of this pull request introduce the two new settings 'tool_opencast/apitimeout' and 'tool_opencast/apiconnecttimeout'. Both to be set in milliseconds. 'tool_opencast/apitimeout' controls 'CURLOPT_TIMEOUT_MS' and 'tool_opencast/apiconnecttimeout' 'CURLOPT_CONNECTTIMEOUT_MS'.

The setting 'tool_opencast/connecttimeout' was dropped because it never had an effect (see #41) and the unit were seconds.

I manually tested the patch with netem/tc

During my work on this patch I discovered the following todos (which should be handled in separate issues/PRs):

  • improve curl error handling
    • no user friendly error message is shown when the API call fails
    • block_opencast: every API call is executed independently, i.g. three calls are made to load the block_opencast video overview page resulting in a page load time of timeout * 3 when there is a problem.
  • revise the $customconfigs parameter handling (currently $customconfigs is an all-or-noting decision, not possible to only set 'apitimeout' for example)

@TamaraGunkel TamaraGunkel self-requested a review March 8, 2022 17:51
@TamaraGunkel TamaraGunkel merged commit 058e5a3 into Opencast-Moodle:master Mar 8, 2022
@TamaraGunkel
Copy link
Contributor

Hey,
thanks for your work!
And you've definitely a point with your todos. We should refactor the code in the future. I'll create an issue for it.

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