Upload with ".tmp" suffix and rename file name after upload #37
Conversation
sakama
force-pushed
the
rename-file-after-upload
branch
2 times, most recently
from
d523562 to
715a8f8
Compare
sakama
force-pushed
the
rename-file-after-upload
branch
from
715a8f8 to
ba43aba
Compare
sakama
changed the title
|
@tvhung83 Could you review this? |
|
Sure, taking a look. |
| @Override | ||
| public Void call() throws IOException | ||
| { | ||
| FileObject remoteFile = newSftpFile(getSftpFileUri(remotePath)); |
There was a problem hiding this comment.
Minor (optional): we can have multiple resources in one try (...):
try (FileObject remoteFile = newSftpFile(getSftpFileUri(remotePath));
BufferedOutputStream outputStream = new BufferedOutputStream(remoteFile.getContent().getOutputStream());
BufferedInputStream inputStream = new BufferedInputStream(new FileInputStream(localTempFile))
) {
logger.info("new sftp file: {}", remoteFile.getPublicURIString());
IOUtils.copy(inputStream, outputStream);
}It can help to ensure all resources are closed properly :)
| private FileObject newSftpFile(final URI sftpUri) throws FileSystemException | ||
| { | ||
| FileObject file = manager.resolveFile(sftpUri.toString(), fsOptions); | ||
| if (file.exists()) { |
There was a problem hiding this comment.
Minor: maybe it's nice to have a warn log, that file is going to be overwritten.
There was a problem hiding this comment.
Thanks. Fixed at 5e6e74f
| file.delete(); | ||
| } | ||
| if (file.getParent().exists()) { | ||
| logger.info("parent directory {} exists there", file.getParent().getPublicURIString()); |
There was a problem hiding this comment.
Ditto: perhaps a warn log is better.
There was a problem hiding this comment.
I think info log is fine because existing parent directory isn't unexpected status.
There was a problem hiding this comment.
Got it, this method is about file, not directory creation, so I'm okay with info log here.
| sftpUtils.uploadFile(tempFile, temporaryFileName); | ||
|
|
||
| Map<String, String> executedFiles = new HashMap<>(); | ||
| executedFiles.put("temporary_filename", fileName + temporaryFileSuffix); |
There was a problem hiding this comment.
Minor: can use temporaryFileName :)
Current implementation has a problem that part of remote files remains at remote when one of tasks failed.
Some users are checking remote file("/path/to/file.txt") existence with job scheduler or something tools for post processing.
I changed implementation to set
temporary_filenameandreal_filenameto Embulk TaskReport atSftpFileOutput.commit().After all tasks completed, plugin will get each file name sets and rename remote files.
Of course, temporary file will be left in remote server "/path/to/file.txt.tmp".
However, this is help full for simple mechanical check.
I tried to add more test cases #39. However, this doesn't work due to permission problem of uploaded files. I think something "hack" is needed.