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.

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 S3 files to local disk instead of holding connections to S3 #157

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

czirker
Copy link
Contributor

@czirker czirker commented Sep 26, 2022

No description provided.

let res;
let rej;
file.localFileReady = new Promise((resolve, reject) => {
res = resolve;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgrantr is there a better way to do this? I just need a promise that I'll resolve somewhere else

Copy link
Contributor

Choose a reason for hiding this comment

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

No. If you don't want everything in the block, what you have done here is what I've done in other places. May want to give it a more descriptive name (like, what does the promise represent) so it's clear what it is used for, but the general approach looks good.

});
}, 10);
downloadQueue.error(function(err, task) {
loggerS3.debug("Download queue error", err, task);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

change this to .error


file.localFilename = s3LocalFileHelper.buildLocalFilePath(file, item.end);

let res;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename these so the are more readable

unlinkSync(file.fullpath);
deleted++;
purgeSize += file.size;
logger.debug("Would delete:", file.fullpath, eidTimestamp, startEidTimestamp, endEidTimestamp, size, maxStorage, eidTimestamp < startEidTimestamp, eidTimestamp > endEidTimestamp && size > maxStorage);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change Would delete to Deleting

let res;
let rej;
file.localFileReady = new Promise((resolve, reject) => {
res = resolve;
Copy link
Contributor

Choose a reason for hiding this comment

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

No. If you don't want everything in the block, what you have done here is what I've done in other places. May want to give it a more descriptive name (like, what does the promise represent) so it's clear what it is used for, but the general approach looks good.

// Connect to as many S3 files as possible while only holding the configured mbs of
// event data in front of it
for (; last_s3_index < items.length && bytes < opts.fast_s3_read_parallel_fetch_max_bytes; last_s3_index++) {
agg_bytes += items[last_s3_index].size; // Accounts for any non S3 data between S3 files
for (; last_s3_index <= index || (last_s3_index < items.length && bytes < opts.fast_s3_read_parallel_fetch_max_bytes); last_s3_index++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@czirker This will fix us not having an S3 file for the given index, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants