Skip to content
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

Refactor FileTransfer.afterSuccess to afterTransfer #2094

Merged
merged 6 commits into from Feb 13, 2014

Conversation

joshmoore
Copy link
Member

In order to prevent the deleting of files when MoveFileTransfer
fails, it was necessary to provide the callback method with
the number of errors.

Further, to have the number of errors properly reported, it was
necessary to notify of a FILE_EXCEPTION on any exception during
uploadFile.

Depending on other import settings, this could trigger a file
upload, which in the case of in-place import is to be avaoided.

/cc @mtbc @pwalczysko

In order to prevent the deleting of files when MoveFileTransfer
**fails**, it was necessary to provide the callback method with
the number of errors.

Further, to have the number of errors properly reported, it was
necessary to notify of a FILE_EXCEPTION on any exception during
uploadFile.

Depending on other import settings, this could trigger a file
upload, which in the case of in-place import is to be avaoided.
@joshmoore
Copy link
Member Author

Assuming ${omero.data.dir} is on a different partition from /tmp/,

 touch /tmp/deleted.fake && bin/omero import -- --transfer=ln_rm /tmp/deleted.fake

gives me this now:

...
2014-02-12 13:41:58,108 6357       [      main] INFO   ormats.importer.cli.LoggingImportMonitor - FILESET_UPLOAD_START
2014-02-12 13:41:58,118 6367       [      main] INFO   mats.importer.transfers.MoveFileTransfer - Transferring /tmp/deleted.fake...
2014-02-12 13:41:58,180 6429       [      main] INFO   ormats.importer.cli.LoggingImportMonitor - FILE_UPLOAD_STARTED: /tmp/deleted.fake
2014-02-12 13:41:58,189 6438       [      main] ERROR  mats.importer.transfers.MoveFileTransfer - transfer process returned 1
2014-02-12 13:41:58,248 6497       [      main] ERROR  mats.importer.transfers.MoveFileTransfer - error in closing raw file store
omero.ResourceError: null
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[na:1.7.0_51]
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:57) ~[na:1.7.0_51]
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[na:1.7.0_51]
        at java.lang.reflect.Constructor.newInstance(Constructor.java:526) ~[na:1.7.0_51]
        at java.lang.Class.newInstance(Class.java:374) ~[na:1.7.0_51]
        at IceInternal.BasicStream.createUserException(BasicStream.java:2615) ~[ice.jar:na]
        at IceInternal.BasicStream.access$300(BasicStream.java:12) ~[ice.jar:na]
        at IceInternal.BasicStream$EncapsDecoder10.throwException(BasicStream.java:3099) ~[ice.jar:na]
        at IceInternal.BasicStream.throwException(BasicStream.java:2077) ~[ice.jar:na]
        at IceInternal.Outgoing.throwUserException(Outgoing.java:538) ~[ice.jar:na]
        at omero.api._RawFileStoreDelM.close(_RawFileStoreDelM.java:466) ~[blitz.jar:na]
        at omero.api.RawFileStorePrxHelper.close(RawFileStorePrxHelper.java:1739) ~[blitz.jar:na]
        at omero.api.RawFileStorePrxHelper.close(RawFileStorePrxHelper.java:1701) ~[blitz.jar:na]
        at ome.formats.importer.transfers.AbstractFileTransfer.cleanupUpload(AbstractFileTransfer.java:123) ~[blitz.jar:na]
        at ome.formats.importer.transfers.AbstractExecFileTransfer.transfer(AbstractExecFileTransfer.java:63) [blitz.jar:na]
        at ome.formats.importer.ImportLibrary.uploadFile(ImportLibrary.java:406) [blitz.jar:na]
        at ome.formats.importer.ImportLibrary.importImage(ImportLibrary.java:467) [blitz.jar:na]
        at ome.formats.importer.ImportLibrary.importCandidates(ImportLibrary.java:270) [blitz.jar:na]
        at ome.formats.importer.cli.CommandLineImporter.start(CommandLineImporter.java:218) [blitz.jar:na]
        at ome.formats.importer.cli.CommandLineImporter.main(CommandLineImporter.java:685) [blitz.jar:na]
2014-02-12 13:41:58,248 6497       [      main] ERROR     ome.formats.importer.cli.ErrorHandler - FILE_EXCEPTION: /tmp/deleted.fake
java.lang.RuntimeException: transfer process returned 1
        at ome.formats.importer.transfers.AbstractExecFileTransfer.exec(AbstractExecFileTransfer.java:137) ~[blitz.jar:na]
        at ome.formats.importer.transfers.AbstractExecFileTransfer.transfer(AbstractExecFileTransfer.java:57) ~[blitz.jar:na]
        at ome.formats.importer.ImportLibrary.uploadFile(ImportLibrary.java:406) [blitz.jar:na]
        at ome.formats.importer.ImportLibrary.importImage(ImportLibrary.java:467) [blitz.jar:na]
        at ome.formats.importer.ImportLibrary.importCandidates(ImportLibrary.java:270) [blitz.jar:na]
        at ome.formats.importer.cli.CommandLineImporter.start(CommandLineImporter.java:218) [blitz.jar:na]
        at ome.formats.importer.cli.CommandLineImporter.main(CommandLineImporter.java:685) [blitz.jar:na]
2014-02-12 13:41:58,249 6498       [      main] ERROR        ome.formats.importer.ImportLibrary - Error on import
java.lang.RuntimeException: transfer process returned 1
        at ome.formats.importer.transfers.AbstractExecFileTransfer.exec(AbstractExecFileTransfer.java:137) ~[blitz.jar:na]
        at ome.formats.importer.transfers.AbstractExecFileTransfer.transfer(AbstractExecFileTransfer.java:57) ~[blitz.jar:na]
        at ome.formats.importer.ImportLibrary.uploadFile(ImportLibrary.java:406) ~[blitz.jar:na]
        at ome.formats.importer.ImportLibrary.importImage(ImportLibrary.java:467) ~[blitz.jar:na]
        at ome.formats.importer.ImportLibrary.importCandidates(ImportLibrary.java:270) ~[blitz.jar:na]
        at ome.formats.importer.cli.CommandLineImporter.start(CommandLineImporter.java:218) [blitz.jar:na]
        at ome.formats.importer.cli.CommandLineImporter.main(CommandLineImporter.java:685) [blitz.jar:na]
2014-02-12 13:41:58,249 6498       [      main] INFO         ome.formats.importer.ImportLibrary - Exiting on error
2014-02-12 13:41:58,249 6498       [      main] ERROR  mats.importer.transfers.MoveFileTransfer - *******************************************
2014-02-12 13:41:58,249 6498       [      main] ERROR  mats.importer.transfers.MoveFileTransfer - 1 errors found! afterSucess not called!
2014-02-12 13:41:58,249 6498       [      main] ERROR  mats.importer.transfers.MoveFileTransfer - The following files will *not* be deleted:
2014-02-12 13:41:58,250 6499       [      main] ERROR  mats.importer.transfers.MoveFileTransfer - /tmp/deleted.fake
2014-02-12 13:41:58,250 6499       [      main] ERROR  mats.importer.transfers.MoveFileTransfer - *******************************************

@mtbc
Copy link
Member

mtbc commented Feb 12, 2014

Perhaps it's worth adding this to a testing scenario.

} else {
String msg = "Unexpected exception thrown!";
log.error(msg, e);
throw new RuntimeException(msg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do throw new RuntimeException(msg, e); so as not to swallow the cause?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, thanks.

@joshmoore
Copy link
Member Author

Here's the set up for testing that failed delete works:

  • sudo touch /home/delete.fake
  • sudo chown jamoore /home/delete.fake
  • chmod a-w /home/delete.fake
  • then import with ln_rm.

What I see with commit 789a55b is:

2014-02-12 13:50:31,025 6159       [l.Client-0] INFO      ome.formats.importer.cli.ErrorHandler - Number of errors: 0
2014-02-12 13:50:31,029 6163       [      main] INFO   mats.importer.transfers.MoveFileTransfer - Deleting source file /home/delete.fake...
2014-02-12 13:50:31,029 6163       [      main] ERROR  mats.importer.transfers.MoveFileTransfer - Failed to remove source file /home/delete.fake
2014-02-12 13:50:31,029 6163       [      main] ERROR  formats.importer.cli.CommandLineImporter - rcode=3 on failed cleanup

@jburel jburel added the dev_5_0 label Feb 12, 2014
@@ -407,17 +407,25 @@ public String uploadFile(final ImportProcessPrx proc,
file, index, srcFiles.length,
proc, this, estimator, cp, buf));
}
catch (IOException e) {
catch (Exception e) {
// Required to bump the error count
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double space

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor fixes pushed.

@mtbc
Copy link
Member

mtbc commented Feb 13, 2014

Works well:

  • normal ln_rm still imports image and deletes file
  • ln_rm across filesystems fails with pile of strange error but doesn't delete file
  • ln_rm delete failure imports image and reports error well.

@mtbc
Copy link
Member

mtbc commented Feb 13, 2014

Good to merge.

joshmoore added a commit that referenced this pull request Feb 13, 2014
Refactor FileTransfer.afterSuccess to afterTransfer
@joshmoore joshmoore merged commit 22545a2 into ome:dev_5_0 Feb 13, 2014
@joshmoore joshmoore deleted the ln-rm-fix branch February 13, 2014 12:37
@joshmoore
Copy link
Member Author

--rebased-to #2125

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants