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

fix #11202: introduce suggestChecksumAlgorithm method to FS API #1653

Merged
merged 8 commits into from Oct 29, 2013

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Oct 23, 2013

The main thing to test is that this breaks nothing: imports still work, and SHA1 hashes are still properly noted in the originalfile DB table. Also, check that the ManagedRepositoryITest unit tests all pass.

--no-rebase as dev_4_4 is limited to only SHA1-160.
Fixes http://trac.openmicroscopy.org.uk/ome/ticket/11202.

# - SHA1-160
# In negotiation with clients, this list is interpreted as being in
# descending order of preference.
omero.checksum.supported=SHA1-160, MD5-128, Murmur3-128, Murmur3-32, Adler-32, CRC-32

Choose a reason for hiding this comment

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

I'd argue that Adler-32 should be at the last position, as it's popularity is low and it's the weakest (yet fastest) in sense of collisions.

@bpindelski
Copy link

Besides the tiny comment, unit tests work ok, import through CLI and Insight too. Looks good to merge.

@jburel
Copy link
Member

jburel commented Oct 24, 2013

@mtbc: no longer possible to import with this PR w/o insight specifying a checksum algorithm

@mtbc
Copy link
Member Author

mtbc commented Oct 25, 2013

Yes: this is why I wanted to get the algorithm specification in earlier (i.e. into 5.0beta2 Insight) rather than later when we have more adopters. The point is to prevent the possibility that the server chooses a checksum algorithm that the client doesn't actually support, especially as the process doesn't complete without the client providing the server the correct hashes. (Also, to prevent the opposite situation.)

Then if, at some later point, what the server supports and what the client's implementation of ChecksumProviderFactory supports don't match too well, we can adjust the code to cope with that without any further API changes.

@mtbc
Copy link
Member Author

mtbc commented Oct 25, 2013

Filed http://trac.openmicroscopy.org.uk/ome/ticket/11577 for the less urgent (not-API-breaking) work to finish this change off.

@mtbc
Copy link
Member Author

mtbc commented Oct 25, 2013

@bpindelski's testing remains valid: I'll revert the revert at some later point!

@joshmoore
Copy link
Member

@jburel : any other issues from insight you're aware of? or all covered under 11577? (And if so, for beta2?)

@jburel
Copy link
Member

jburel commented Oct 29, 2013

All "upload" will need to be carefully tested, can be done for beta2 just need to have a clear list. (I can prepare and check)

@joshmoore
Copy link
Member

👍

joshmoore added a commit that referenced this pull request Oct 29, 2013
fix #11202: introduce suggestChecksumAlgorithm method to FS API
@joshmoore joshmoore merged commit b9becb3 into ome:develop Oct 29, 2013
@mtbc mtbc deleted the 11202-negotiate-hasher branch October 30, 2013 08:51
mtbc added a commit to mtbc/openmicroscopy that referenced this pull request Nov 6, 2013
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

4 participants