keith-turner commented on code in PR #36:
URL:
https://github.com/apache/accumulo-classloaders/pull/36#discussion_r2687678852
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LocalStore.java:
##########
@@ -290,12 +292,17 @@ private Path storeResource(final Resource resource)
throws IOException {
}
}
+ private static final int DL_BUFF_SIZE = 1024 * 1024;
+
private void downloadFile(FileResolver source, Path tempPath, Resource
resource) {
// CREATE_NEW ensures the temporary file name is unique for this attempt
- // SYNC ensures file integrity on each write, in case of system failure
- try (var in = source.getInputStream();
- var out = Files.newOutputStream(tempPath, CREATE_NEW, WRITE, SYNC)) {
+ // SYNC ensures file integrity on each write, in case of system failure.
Buffering minimizes
+ // system calls te read/write data which minimizes the number of syncs.
+ try (var in = new BufferedInputStream(source.getInputStream(),
DL_BUFF_SIZE);
+ var out = new BufferedOutputStream(Files.newOutputStream(tempPath,
CREATE_NEW, WRITE, SYNC),
+ DL_BUFF_SIZE)) {
in.transferTo(out);
+ out.close();
Review Comment:
That does not feel like it guaranteed to always work. The buffer will
significantly reduce the number of syncs done w/ the current code. Also in the
past I have seen good result when wrapping file and network streams w/ buffered
streams, so adding the buffers is probably generally good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
