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
DM-25881: Add extra logging to S3 datastore #322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general but I have a couple of quick changes that need to happen.
self.client.put_object(Bucket=location.netloc, Key=location.relativeToPathRoot, | ||
Body=serializedDataset) | ||
log.debug("Wrote file directly to %s", location.uri) | ||
log.debug(f"Successfully wrote file directly to {location.uri}") | ||
except NotImplementedError: | ||
with tempfile.NamedTemporaryFile(suffix=location.getExtension()) as tmpFile: | ||
formatter._fileDescriptor.location = Location(*os.path.split(tmpFile.name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need a log message around line 225 (new) where we upload this temporary file. This is the code path that will trigger for fits files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding line 225 (new), before it attempts the put_object via a temporary file.
@@ -113,6 +113,7 @@ def _artifact_exists(self, location: Location) -> bool: | |||
exists : `bool` | |||
True if the location can be found, false otherwise. | |||
""" | |||
log.debug(f"Checking if file exists: '{location.uri}''") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general comment for debug log messages we tend to use the %s
deferred formatting approach following the notion that there is no point stringifying location.uri
if the debug message is not going to be issued. So all these should ideally be changed to:
log.debug("something or other %s", location.uri)
fe9ac96
to
a2aa2bd
Compare
No description provided.