-
Notifications
You must be signed in to change notification settings - Fork 8
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
Download files in monitor #89
Conversation
pkg/monitor/monitor.go
Outdated
if _, err := os.Stat(basePath); os.IsNotExist(err) { | ||
log.Debugf("Temporary base path '%s' doesn't exist - creating", basePath) | ||
if err = os.MkdirAll(basePath, 0755); err != nil { | ||
log.Errorf("Cannot created temporary base path: %s", err.Error()) |
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.
nit: s/created/create
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.
Aaah, the english language....
pkg/monitor/monitor.go
Outdated
log.Debugf("Using temporary base path: %s", basePath) | ||
fileEntry, err := m.FilesClient.Download(&file, basePath) | ||
if err != nil { | ||
log.Errorf("Filed to download %s: %s", file.Path, err) |
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.
nit: s/Filed/File
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.
Hmm, this should have been failed
@@ -247,11 +247,6 @@ func NewReportRunner(cfg *config.Config, dbConn *gorm.DB, sf common.SalesforceCl | |||
return nil, err | |||
} |
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.
Do you still need any of the above code?
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.
Yes, that's the working directory of the shell script for the processor.
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.
Ack, just seemed like the new code was doing the same thing.
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.
The basepath
corresponds to the mount point in docker-compose
where the shared folder is mounted on both containers. That needs to be set in both configuration files (which is a little hackish at the moment and needs some cleaning). The processor then creates a directory underneath that where it unpacks the sos reports and runs hotsos. That step is as it was before.
884cd27
to
dc4eea8
Compare
if err = os.MkdirAll(basePath, 0755); err != nil { | ||
log.Errorf("Cannot create temporary base path: %s", err.Error()) | ||
} | ||
} |
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.
Ok, one more question. Here the directory is getting creating after the download, should that be before?
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.
Yes, that's a good point.
@@ -247,11 +247,6 @@ func NewReportRunner(cfg *config.Config, dbConn *gorm.DB, sf common.SalesforceCl | |||
return nil, err | |||
} |
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.
Ack, just seemed like the new code was doing the same thing.
This change tried to address potential race conditions between Athena and the file mover. The idea is to download new sos reports in the monitor and park them in a common filesystem. The processor eventually proceeds with processing the files from the common filesystem. Signed-off-by: Nicolas Bock <[email protected]>
This change tried to address potential race conditions between Athena
and the file mover. The idea is to download new sos reports in the
monitor and park them in a common filesystem. The processor eventually
proceeds with processing the files from the common filesystem.
Signed-off-by: Nicolas Bock [email protected]