-
Notifications
You must be signed in to change notification settings - Fork 85
chore(benchmarking): Putting temp files in try with resource for cleanup #2208
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
Conversation
| // Create the file to be uploaded and fill it with data | ||
| TmpFile file = DataGenerator.base64Characters().tempFile(tempDirectory, objectSize); | ||
| BlobInfo blob = BlobInfo.newBuilder(bucketName, file.toString()).build(); | ||
| try(TmpFile file = DataGenerator.base64Characters().tempFile(tempDirectory, objectSize)) { |
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 only actual changes here are the additions of the try here
| created) | ||
| .formatAsCustomMetric()); | ||
| for (int i = 0; i <= StorageSharedBenchmarkingUtils.DEFAULT_NUMBER_OF_READS; i++) { | ||
| try (TmpFile dest = TmpFile.of(tempDirectory, "prefix", "bin")) { |
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.
and here, the rest is formatting.
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.
Primary change looks okay, but there is a follow up for the data generation that will need to be addressed.
| // Create the file to be uploaded and fill it with data | ||
| TmpFile file = DataGenerator.base64Characters().tempFile(tempDirectory, objectSize); | ||
| BlobInfo blob = BlobInfo.newBuilder(bucketName, file.toString()).build(); | ||
| try (TmpFile file = DataGenerator.base64Characters().tempFile(tempDirectory, objectSize)) { |
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.
you don't want to use the base64 character set here for generate. It creates highly compressible and dedupeable data which Gcs sometimes will optimize for leading to skewed data.
You can either use the existing rand or port the devUrandom implementation from the existing benchmarks into this repos version.
it's fine to do this in a follow up pr.
No description provided.