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

RQ Job Timeout Handling #223

Closed
wants to merge 13 commits into from
Closed

Conversation

JVickery-TBS
Copy link
Contributor

fix(logic): close temp file on rq timeout;

  • Always close the temp file on exception handling.
  • Added handling for the rq job timeout.

- Always close the temp file on exception handling.
- Added handling for the rq job timeout.
ckanext/xloader/jobs.py Outdated Show resolved Hide resolved
- Moved try/catch for `tabulator_load` rq timeout up.
ckanext/xloader/jobs.py Outdated Show resolved Hide resolved
@duttonw
Copy link
Collaborator

duttonw commented Oct 15, 2024

@JVickery-TBS i'd like the idea but with the tests failing, I can't look at merging this. Also unsure about what @ThrawnCA picked up on another path that is not caught correctly.

@duttonw
Copy link
Collaborator

duttonw commented Nov 23, 2024

Hi @JVickery-TBS ,

I'd like to incorperate this into xloader. Are you able to rebase to master so that we have the tests fixed.

Regards,

@duttonw

@JVickery-TBS
Copy link
Contributor Author

@duttonw hey! SORRY! I have been swamped with work and general life stuff. (But I did just get married so yay!)

Hoping to get back into all this XLoader and Validation stuff in the coming weeks.

# Conflicts:
#	ckanext/xloader/jobs.py
### RESOLVED.
- Changed positioning of try/catch for job timeout to handle all of the type guessing retries.
- Log level for `ckanext.xloader`
- No alias import to fix try/catch.
- TMPDIR fixes.
- Simplify test with specific file suffix check.
- Typo in logging.
ckanext/xloader/jobs.py Outdated Show resolved Hide resolved
- More attempts...
@JVickery-TBS
Copy link
Contributor Author

@duttonw lemme know if this one is good, and I can make a squashed version

JVickery-TBS added a commit to JVickery-TBS/ckanext-xloader that referenced this pull request Dec 9, 2024
@JVickery-TBS
Copy link
Contributor Author

@duttonw @ThrawnCA squashed version: #239 (and added to the changelog file too)

except FileNotFoundError:
pass
logger.warning('Job timed out after %ss', RETRIED_JOB_TIMEOUT)
raise JobError('Job timed out after {}s'.format(RETRIED_JOB_TIMEOUT))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're generating this string anyway, might as well use it for the logger as well.

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