combats uploader race condition #4
Conversation
Putting in a CONDITIONAL wait to combat a race condition in the distributed file system. It first checks to see if the file exists. If it doesn't, it waits .1 seconds, if it still doesn't, it waits 1.0 seconds. Then it raises high holy hell about the file not existing. So if it does indeed exist, we've only incurred the cost of running a single existence check and an if, so it will only do the wait if it already HAS hit the race condition.
doing error reporting correctly via logging.exception (instead of print() )
Proper log message for 0.7.3
uploader/uploader.py
Outdated
| if not os.path.exists(file): | ||
| time.sleep(1) | ||
| if not os.path.exists(file): | ||
| logging.exception(f"ERROR! File {file} does not exist for uploader even after a wait!") |
There was a problem hiding this comment.
Use logging.exception when you want to print the stack trace for the exception. Since there is no exception at this point, no need to do this. Additionally we will print the raised exception on line 44 so no need to print it twice, just the raise is good enough.
There was a problem hiding this comment.
I'm trying to parse this out.
I think you're saying that when the checks for the file are exhausted, it's not appropriate to call "logging.exception". But are you saying to leave the "raise ValueError"?
I just want to make sure that if the uploader fails because the file it's been told to upload doesn't exist, then what file it's looking for is explicitly stated and that "does not exist", so that the user knows it's not a network problem or a queue problem or something.
There was a problem hiding this comment.
Maybe a point of clarity: The logging.exception() is being called in the try: block that started on line 30. At line 44, no exception as been raised that logging.exception() can use in its massaging.
As a loose rule, logging.exception() should only be called in except: blocks.
The except: block is where the program skips to when an exception is raised and thus there is a traceback availbile that logging.exception() can "read" and populate the logging message with. Notice this is what is happening between line 51 and 56
There was a problem hiding this comment.
@robkooper I used the github interface to apply the patch.
Once I got the file back, I don't think it was right. So I spliced the two trees together the way I think you were thinking, which I agree with; it's much more compact. And I removed the logging.exception but left the raise ValueError.
Let me know if that matches what you were thinking. If you think this looks Ok, we should probably exercise the uploader both normally, without the race condition, and then artificially trip it so that it definitely fires, so to make sure it flows correctly.
There was a problem hiding this comment.
Ah, thanks, that helps a lot.
Unfortunately I uploaded a new commit just as you posted that comment. Could you glance at the new commit? The patch tool in github kind of mangled the test loops and the new commit cleans them up.
Also I think I've fixed my original misplaced "except" statement.
There was a problem hiding this comment.
Everything looks correct to me.
Replacing tree if ifs (for os.path.exists for the target) with a while(not exists) loop with a counter. Makes it more flexible and more compact. Co-authored-by: Rob Kooper <[email protected]>
I don't think the github patch applier got the final version of the code right. I think this is what Rob intended with his revised while loop replacing the couple of ifs I had initially written.
Putting in a CONDITIONAL wait to combat a race condition in the distributed file system. It first checks to see if the file exists. If it doesn't, it waits .1 seconds, if it still doesn't, it waits 1.0 seconds. Then it raises high holy hell about the file not existing.
So if it does indeed exist, we've only incurred the cost of running a single existence check and an if, so it will only do the wait if it already HAS hit the race condition.