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

add file size hash; make image acquisition date optional #2847

Merged
merged 36 commits into from
Jul 31, 2014

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Jul 22, 2014

Adds the File-Size-64 hasher, adjusts Adler-32 and CRC-32 to use Guava implementations, and tweaks some import log retrieval.

To test,

  • without this PR, try importing some images with various hashers set, including Adler-32 and/or CRC-32
  • upgrade to OMERO5.1DEV__7
  • import some of the same images with the same hashers, and also File-Size-64
    • and check that you can view the import logs
  • verify that
    • the same images with the same hasher have the same hash regardless of if they were imported with or without this PR
    • for files imported with File-Size-64, their hash is a small-endian representation of their size
  • for images imported after the upgrade, check that those that should have acquisition dates do (e.g., SVS) and those that shouldn't don't (e.g., PNG) (update: the latter now requires do not insert acquisition dates by default unless from metadata bioformats#1235)
    • check that null acquisition dates do not cause obvious problems: for instance, that images can still be viewed, exported, etc.

--rebased-from #2741 and fixes http://trac.openmicroscopy.org.uk/ome/ticket/12465

mtbc added 27 commits July 21, 2014 12:59
Conflicts:
	components/model/resources/mappings/acquisition.ome.xml
Conflicts:
	components/common/src/ome/util/checksum/FileSizeChecksumProviderImpl.java
Conflicts:
	components/common/test/ome/util/checksum/FileSizeChecksumProviderImplTest.java
Conflicts:
	components/common/src/ome/util/checksum/AbstractChecksumProviderReverseEndian.java
@mtbc
Copy link
Member Author

mtbc commented Jul 22, 2014

This PR needs a breaking label at present.

@mtbc
Copy link
Member Author

mtbc commented Jul 23, 2014

Okay to remove the breaking label now or should @jburel review before inclusion in merge build?

@joshmoore
Copy link
Member

The DB changes are the ones that worry me the least, so happy to remove.

@jburel
Copy link
Member

jburel commented Jul 24, 2014

@mtbc, @qidane : we will need to review the XML mock Factory in bf repository where acquisition is set to be not null. not a blocker for that PR.

@jburel
Copy link
Member

jburel commented Jul 24, 2014

@mtbc: checking other sections of code I can think of before merging.

@jburel
Copy link
Member

jburel commented Jul 24, 2014

@mtbc: in ModelMockFactory, do we still want to set the acquisition time in simpleImage.
Many tests pass 0 as a parameter.

@@ -188,6 +189,7 @@ public void init(Helper helper) {
try {
sf = reg.getInternalServiceFactory(
sessionUuid, "unused", 3, 1, clientUuid);
MetadataTools.setDefaultDateEnabled(false);
Copy link
Member

Choose a reason for hiding this comment

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

@mtbc: in discussing with @melissalinkert probably best to remove this, and in favor for making the default in Bio-Formats false. Once you've opened that PR, they can be linked together with a --depends-on.

This reverts commit fc7b561
because the default in Bio-Formats is to be toggled.
@mtbc
Copy link
Member Author

mtbc commented Jul 24, 2014

@jburel: good idea, done in #2870.

@jburel
Copy link
Member

jburel commented Jul 24, 2014

@mtbc, @qidane: my mistake, I checked the XMLModelMockObjects (B-F/specification), the acquisition date is not set. So nothing to change there.

@jburel
Copy link
Member

jburel commented Jul 25, 2014

In components/server/src/ome/logic/PixelsImpl.java
The createImage method sets the Acquisition Date to new Timestamp(new Date().getTime()));. This should be removed.

@bpindelski
Copy link

Tested with a local server@HEAD of develop which later got upgraded to snoopy's merge build.

  • CRC-32 and Adler-32 checksums are the same for images imported pre- and post-DB upgrade,
  • file size hasher indeed stores the images size in little-endian order,
  • acquisition date appears for SVS but doesn't for DV or png.

I can't see any issues with the diff, so I'd say this is good to merge.

@jburel
Copy link
Member

jburel commented Jul 29, 2014

@mtbc: tested with some other images after yesterday stress.
Both images imported against octopus and trout. Same date displayed i.e. date from the files.
octopus
trout

@jburel
Copy link
Member

jburel commented Jul 29, 2014

I will do some final tests tomorrow am.

@jburel
Copy link
Member

jburel commented Jul 30, 2014

@mtbc, @dominikl: The fact that the acquisition date can now be null will lead to NPE in the recent search code.
I am currently fixing a time issue related to search in dev_5_0 so the problem can be fixed in another PR since this is already large.

@jburel
Copy link
Member

jburel commented Jul 30, 2014

unless anybody wants to have a final check, we should probably merge the PR.

@mtbc
Copy link
Member Author

mtbc commented Jul 30, 2014

It's been a week and listed in stand-up a few times: I think that everybody who might have wanted to probably already has. (-:

@sbesson
Copy link
Member

sbesson commented Jul 31, 2014

See #2892. Integration tests using fake image with and without acquisition dates pass. Ready to merge.

jburel added a commit that referenced this pull request Jul 31, 2014
…date

add file size hash; make image acquisition date optional
@jburel jburel merged commit 1d21578 into ome:develop Jul 31, 2014
@mtbc mtbc deleted the file-size-hash-and-no-acquisition-date branch July 31, 2014 08:59
@sbesson sbesson added this to the 5.1.0-m1 milestone Oct 14, 2014
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

5 participants