icatproject / topcat

GUI to work with multiple ICAT and IDS servers.
Other
1 stars 9 forks source link

StatusCheck should only expire downloads on TopcatExceptions #462

Closed brianritchie1312 closed 4 years ago

brianritchie1312 commented 4 years ago

Originally, StatusCheck only set a download's status to Expired when a TopcatException was encountered; all other exceptions were logged but ignored. However, in those cases, the lastChecks timestamp was not updated, which could lead to StatusCheck re-querying the IDS once per second. The fix for issue #449 was to have Topcat expire the download in response to any exception.

There has since been some evidence (a download status changing from Preparing to Expired due to an IOException that would have previously been ignored) that it might be better to return to expiring downloads only on TopcatExceptions. However, for all other exceptions the lastChecks timestamp should be updated, so that StatusCheck only repeats the test after the configured interval.

brianritchie1312 commented 4 years ago

Note that the MockIdsClient defined within StatusCheck test is set to throw a TopcatException when "failure mode" is enabled. For this change, strictly there should be two failure modes; the second would throw a different exception, and there should be a new test that in the second mode StatusCheck does not expire the download, but doesn't immediately call the mock client again. Such a test may be tricky: StatusCheckTest currently does no interval testing. (I was also worried that the real StatusCheck polling may interfere with any attempts to do so, but the test configuration's properties include (or should include) a flag that disables the polled method.)

brianritchie1312 commented 4 years ago

A few comments on StatusCheckTest: