Throw error on empty file #334
Conversation
(This change is for the current 6.0.2 version)
| @@ -77,7 +77,11 @@ export class FileDescriptor extends AbstractDescriptor implements FileDescriptor | |||
| private async _calculateSize(modifier: number = 1): Promise<void> { | |||
| if (this.size == null || this.size <= 0) { | |||
| const asyncStat = promisify(fs.stat); | |||
| this.size = (await asyncStat(this.file as string)).size * modifier; | |||
| const fileSize = await asyncStat(this.file as string); | |||
| if (fileSize.size <= 0) { | |||
There was a problem hiding this comment.
use the same comparison to all
There was a problem hiding this comment.
one time you have <= 0 and above you have === 0
| } | ||
|
|
||
| if (uploadResponse?.errorCode != null) { |
There was a problem hiding this comment.
Doesnt this stop retries ?
There was a problem hiding this comment.
No, the only way we signal we want a retry is to throw an error. All errors retry, we don't support permanent errors it seems
There was a problem hiding this comment.
Oh right. There is no such concept in our non-.net SDKs - maybe a little bit in Java ..
There was a problem hiding this comment.
This seems like a big thing - when does it happen that response doesnt throw but has an error ?
There was a problem hiding this comment.
It should throw on any 400 or 500 code
| if (fileSize.size <= 0) { | ||
| throw Error("Empty file."); | ||
| } | ||
| this.size = fileSize.size * modifier; |
There was a problem hiding this comment.
dont remember we had this modifier thing - did someone add this here and not to browser code ?
There was a problem hiding this comment.
Yes, this was the previous version of the method:
private async _calculateSize(modifier: number = 1): Promise<void> {
if (this.size == null || this.size <= 0) {
const asyncStat = promisify(fs.stat);
this.size = (await asyncStat(this.file as string)).size * modifier;
}
}
```
According to git blame, you added this last year. Not sure why it's not in browser
There was a problem hiding this comment.
so you want to add this to browser code ? should i?
| @@ -50,4 +56,15 @@ export class FileDescriptor extends AbstractDescriptor implements FileDescriptor | |||
| getCompressionSuffix() { | |||
| return this.compressionType ? `.${this.compressionType}` : ".gz"; | |||
| } | |||
|
|
|||
| private _calculateSize(modifier: number = 1): void { | |||
| if (this.size == null || this.size <= 0) { | |||
There was a problem hiding this comment.
actually i dont think this works - i dont think size here is ever null .. as we take the file size as the size by default
| @@ -50,4 +56,14 @@ export class FileDescriptor extends AbstractDescriptor implements FileDescriptor | |||
| getCompressionSuffix() { | |||
| return this.compressionType ? `.${this.compressionType}` : ".gz"; | |||
| } | |||
|
|
|||
| private _calculateSize(modifier: number = 1): void { | |||
There was a problem hiding this comment.
Why is this a fileDescriptor thing to do ? its relevant to all other descriptors ..
Its easier when we align all this king of logic to our other SDKs - this way we can make sure we dont have loose ends
Co-authored-by: Asaf Mahlev <asafmahlev@microsoft.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ariel Yehezkely <aryehezk@microsoft.com> Co-authored-by: alonadam <96700455+alonadam@users.noreply.github.com> Fix streaming ingestion (#341)
(This change is for the current 6.0.2 version)