Inplace Import via SymlinkFileTransfer (See #11573) #1989
Conversation
|
I've tested this using |
|
Ack. You're right, @ximenesuk . My git rebase went haywire. I'll piece that back apart when I do the rebasing. Just merging get-fild-id now. |
The uploadFile method has a number of responsibilities
that were not easily modifiable. Rather than insert
further `if (symlink) {} then {}` complications, its
been extracted behind the new `FileTransfer` interface
to permit others to inject their own logic.
The number of parameters passed to the `transfer` method was daunting at best. Further, much of the uploadFile method was doing logging so that moving the execution into the state class, the FileTransfer implementors can be ignorant of most of the arguments.
The rest of the reusable logic from uploadFile has now been moved to AbstractFileTransfar and re-used by the new SymlinkFileTransfer class. Several remote-access methods were needed in the ImportLibrary itself, though these could perhaps live in a more general gateway-style utility.
The following options are now all available for choosing which transfer method to use: ``` bin/omero import -t upload bin/omero import --transfer upload bin/omero import -t symlink bin/omero import -- -t symlink bin/omero import -t your.class.name.Here ```
|
Commit split and rebased onto |
|
I've given this rebased PR a quick run through, importing in both transfer modes for both single and multi-file formats. I've not come across any problems, imports run okay, images are viewable, etc. Is there anything deeper needs doing? |
|
I don't think so, @ximenesuk. At this point, we probably need more differentiated testing (screens, etc) a la @pwalczysko and usability questions like "is it clear to users doing this that they have no data guarantees?" (/cc @gusferguson ) Considering this requires: 1) Shelling into the server and 2) manually adding the flag at the moment, I'd say it's fairly safe. @jburel, thoughts? |
|
@joshmoore might it also be worth considering whether to add Is there any plan to have a |
|
The implementation would be quite straight-forward but less safe for the end user. Maybe we require: |
|
My plan is to push a minimal refactoring to this branch to make it easy to implement |
A simple `mv` command is not possible since 1) the file must be present for subsequent readds and 2) it's not safe to remove the file until the import has completed successfully. Now, users of the `FileTransfer` API have the responsibility of calling `afterSuccess` once the `ImportLibrary` has done its work. An exception may be thrown if cleanup is not possible.
|
Mentioned refactoring has been pushed. Note: the commands for each transfer have slightly changed:
PR description updated. |
|
Also updated the CLI help: |
|
Thanks @joshmoore I'll review this if there are no further changes expected imminently. |
| protected File getLocalLocation(OriginalFile root, OriginalFile ofile) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| sb.append(root.getPath().getValue()); | ||
| sb.append("/"); |
There was a problem hiding this comment.
I'm tempted to use in the constructor: if (windows) { throw ... }
There was a problem hiding this comment.
(-: Less to fix when we get this working for Windows. Might want to wait for Java 7, but this should be doable on NTFS?
|
Using Importing an entire lei folder containing several sets of files the only files that are deleted are the |
|
I ran through the various options with the dv files, all upload methods look to work okay, and confirmed the delete issue with lei files but I'll leave any wider review for now. |
```
$ bin/omero import -- --transfer-help
...
Advanced arguments:
--transfer=ARG File transfer method
Examples:
-t upload # Default
-t ln # Use hard-link. Locally only!
-t ln_rm # Caution! Hard-link followed by rm. Locally only!
-t ln_s # Use symlnk. Locally only!
-t some.class.Name # Use a class on the CLASSPATH
ex. importer-cli --tranasfer=ln_s foo.tiff
Report bugs to <ome-users@lists.openmicroscopy.org.uk>
```
|
Fix for MIF and hiding of advanced usage pushed. |
|
The delete now works okay. Is it now worth merging this (once the typo is fixed) even if we readdress what is exposed and how later? |
With this change, an annotation is added to the `Fileset` object on import describing which `FileTransfer` implementation was used. Use of the default `UploadFileTransfer` is ignored; otherwise, the FQN is stored as the content of the annotation. If we decided to move to subclasses of `Fileset` these values should suffice. Representative output after an import includes: ``` ome2=> \x ome2=> select discriminator, id, ns, textValue from annotation order by id desc limit 2; -[ RECORD 1 ]-+--------------------------------------------------- discriminator | /type/OriginalFile/ id | 3102 ns | openmicroscopy.org/omero/import/logFile textvalue | -[ RECORD 2 ]-+--------------------------------------------------- discriminator | /basic/text/comment/ id | 3101 ns | openmicroscopy.org/omero/import/fileTransfer textvalue | ome.formats.importer.transfers.SymlinkFileTransfer ```
|
Typo fixed and an annotation added for the file transfer typo. |
|
This all looks good. |
|
Thanks for all the back and forth, @ximenesuk. Merging, though there may be some other small requests before |
Inplace Import via SymlinkFileTransfer (See #11573)
|
--rebased-to #2020 |
https://trac.openmicroscopy.org.uk/ome/ticket/11573
This PR implements the same logic as the
InPlaceImporter.pyscript from gh-1929 but in Java. The only thing that is currently not possible is the remote invocation that a script provides. But considering there are security issues when using the script, this is the safer intermediate solution.To test:
As with gh-1929, the goal is that it's possible to import all files that were previously importable, but the files of the fileset under
/OMERO/ManagedRepositoryshould be symlinks not the original files. You will need to shell into the machine running OMERO, and import via the CLI:The key lines to look for in the output:
To do:
dev_5_0once RawFileStore.getFileId #1987 is merged. (No commit comments until then, please)Once this is merged, the GUI importer can be updated to also symlink. /cc @jburel