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.

Sign up for GitHub

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

combats uploader race condition #4

Merged
merged 5 commits into from
Jun 27, 2024
Merged

combats uploader race condition #4

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e8ee20f
if-->wait race condition
craigsteffen May 13, 2024
f8ed237
print -> logging.exception
craigsteffen May 13, 2024
46ec7c7
update for 0.7.3
craigsteffen May 13, 2024
f588362
if if -> while not w/ counter
craigsteffen Jun 26, 2024
d81fd8e
clean up file check if tree
craigsteffen Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## [0.7.3] - 2024-05-13

### Added
- Added conditional wait to combat potential parallel file system race condition when pipeline components are all running synchronously. Only waits when file that should exist doesn't exist.

## [0.7.2] - 2024-05-03

### Fixed
Expand Down
8 changes: 8 additions & 0 deletions uploader/uploader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import requests
from requests.exceptions import RequestException
import threading
import time

# rabbitmq uri
rabbitmq_uri = os.getenv("RABBITMQ_URI", "amqp://guest:guest@localhost:5672/%2F")
Expand All @@ -30,6 +31,13 @@ def run(self):
data = json.loads(self.body)
file = os.path.join("/output", data['cdr_output'])
logging.debug(f"Uploading data for {data['cog_id']} from {file}")
if not os.path.exists(file):
craigsteffen marked this conversation as resolved.
Show resolved Hide resolved
time.sleep(.1)
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!")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robkooper

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asaxton

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Everything looks correct to me.

raise ValueError(f"File {file} does not exist for uploader!!!")
# only upload if less than certain size
if os.path.getsize(file) > max_size * 1024 * 1024: # size in bytes
raise ValueError(f"File {file} is larger than {max_size}MB, skipping upload.")
Expand Down
Loading