Skip to content

[CCAP-670] Enhance file conversion service to retry up to 10 times if LibreOffice fails#658

Merged
cram-cfa merged 1 commit intomainfrom
marc-CCAP-670
Feb 12, 2025
Merged

[CCAP-670] Enhance file conversion service to retry up to 10 times if LibreOffice fails#658
cram-cfa merged 1 commit intomainfrom
marc-CCAP-670

Conversation

@cram-cfa
Copy link
Copy Markdown
Collaborator

Issue tracking number 🔗

Description of change ✍️

Priority 🥇

Effect on other applications using FFB 🌊

Testing

✅ Checklist before requesting a review

  • Does the new code follow our preferred coding
    style
    ?
  • Does the code include javadocs, where necessary?
  • Have tests for this feature been added / updated?
  • Has the readme been updated?

// converted file. We can retry in this situation.
log.warn("Unable to find PDF file {}, will retry.", pdfFile.getAbsolutePath());
return convertOfficeDocumentToPDF(file);
throw new RuntimeException("PDF conversion failed. Unable to find PDF file " + pdfFile.getAbsolutePath() + " after " + numberOfRetries + " retries. Final exit code: " + exitCode);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens when this exception is thrown after 10 retries?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It gets all the way up to the FileController:

log.error("Error converting file {} to PDF", userFileId, e);

And then nothing happens except logging that nothing was converted: https://github.com/codeforamerica/form-flow/blob/bd983f2a93ceacbb5fc1df91c05c3f7d11afe4a5/src/main/java/formflow/library/FileController.java#L280C19-L280C42

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it bubbling up to the FileController makes sense, but maybe we should make a slack alert in Datadog if this happens so we can be alerted, as my understanding is if this happens, then it's possible we don't end up sending a supporting document with an application?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yep, there's already a datadog alert for all the warns/errors/interesting-infos and they are going to #snlab-il-doc-conversion-alerts

I'll pare those down into more interesting/useful alerts and send them to the normal alerts channel once we're done testing and we don't care about every single logging statement!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok sweet! Thanks for being thorough!

@cram-cfa cram-cfa merged commit 3dbfe6c into main Feb 12, 2025
5 checks passed
@cram-cfa cram-cfa deleted the marc-CCAP-670 branch February 12, 2025 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants