Skip to content

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.

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

3 participants