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

Set /pdf requests per node 8 to 4, adds retry. #402

Merged
merged 4 commits into from
Oct 11, 2023
Merged

Conversation

bdc34
Copy link
Contributor

@bdc34 bdc34 commented Oct 10, 2023

This:

  1. decreases the concurrent per webnode /pdf requests from 8 to 4
  2. adds retry to /pdf reqeusts
  3. adds retry to upload to GS
  4. increases the wait time for a pdf to be built from 3 minutes to 5 minutes.
  5. Adds a log msg on successful GET /pdf

Both retry's use the default settings: initial delay of 1 sec, max delay of 60 sec, multiplier of 2 and a timeout of 120 sec.

The docs say timeout is "Timeout: the maximum duration of time after which a certain operation must terminate (successfully or with an error). The countdown begins right after an operation was started. For example, if an operation was started at 09:24:00 with timeout of 75 seconds, it must terminate no later than 09:25:15."

@@ -262,6 +278,31 @@ def path_to_bucket_key(pdf) -> str:
raise ValueError(f"Cannot convert PDF path {pdf} to a GS key")


@retry.Retry(predicate=retry.if_exception_type(PDF_RETRY_EXCEPTIONS))
def get_pdf(session, pdf_url) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moves the GET /pdf out to use a retry on it.

Copy link
Contributor

@cbf66 cbf66 left a comment

Choose a reason for hiding this comment

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

It'd be nice if the log messages could record the time it took to make each PDF, but we can add that tomorrow.

Copy link
Contributor

@DavidLFielding DavidLFielding left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

Keep in mind, when not in redirect mode, that other requests will also trigger compilations. This may result in some 503 errors.

The 503 handling currently does not appear to do anything to slow things down. Will not be an issue when requests and max processes are in sync. Other user requests may result in compilations and 503 errors.

@bdc34
Copy link
Contributor Author

bdc34 commented Oct 11, 2023

These changes look good to me.

Keep in mind, when not in redirect mode, that other requests will also trigger compilations. This may result in some 503 errors.

The 503 handling currently does not appear to do anything to slow things down. Will not be an issue when requests and max processes are in sync. Other user requests may result in compilations and 503 errors.

The @retry.Retry() on line will cause the retry to happen on a 503 but will only do the retry after a pause. On the first 503 it will be a 1 sec pause, then a 2 sec. then a 4 sec etc. There are parameters to tune how these pauses happen.

There is no mechanism in place in this code for slowing down beyond that. I think this should be fine since returning the 503 from /pdf is should be low cost and low load.

@bdc34 bdc34 merged commit 6ea67d7 into develop Oct 11, 2023
3 of 4 checks passed
@bdc34 bdc34 deleted the sync-per-node-8-to-4 branch October 11, 2023 19:37
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