[CAT-799] Timeout Parameter Not Set Up as Expected #273
Conversation
indico/types/utils.py
Outdated
| def run(self): | ||
| if self.timeout == 0: | ||
| raise IndicoTimeoutError(self.timeout) | ||
| signal.signal(signal.SIGALRM, self._handler) |
There was a problem hiding this comment.
oh this is really slick! I dig it.
Reading the docs for the signal library, it looks like signal.SIGALARM is only available on Unix. I think we want this SDK to be usable on Windows as well. Is there a way to implement this that's more OS agnostic?
There was a problem hiding this comment.
ah yep will do!
There was a problem hiding this comment.
Looks like signal is just not overall a good choice as it's not Windows friendly in general. Just parsed out current timing logic into Timer and fixed the double time issues
|
nice! this looks good to me, but I'd hold off on merging until @goatrocks takes a look |
indico/queries/jobs.py
Outdated
| @@ -53,7 +52,7 @@ class JobStatus(RequestChain): | |||
| Args: | |||
| id (int): id of the job to query for status. | |||
| wait (bool, optional): Wait for the job to complete? Default is True | |||
| timeout (float or int, optional): Timeout after this many seconds. | |||
There was a problem hiding this comment.
Nice work on the fix + updating the tests!
Only feedback here - there's an impact to changing the interface/accepted types -- while you could submit an int for a float, if users are submitting floats and upgrade after we narrow the accepted types to int, they will have to change their scripts.
If it's possible to accept floats here we should still. Thank you!
Summary
Type of change
Checklist: