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

Change compute checksums to digest in single pass. #1100

Merged
merged 1 commit into from
Sep 18, 2023
Merged

Conversation

justinlittman
Copy link
Contributor

refs #1074

Why was this change made? 🤔

Need for speed.

How was this change tested? 🤨

⚡ ⚠ If this change has cross service impact, including writes to shared file systems, run integration tests and/or test in [stage|qa] environment, in addition to specs. ⚡

Unit

@lwrubel lwrubel merged commit a15c040 into main Sep 18, 2023
@lwrubel lwrubel deleted the t1074-checksum branch September 18, 2023 11:48
@mjgiarlo
Copy link
Member

@justinlittman do you think we should consider extracting this to the assembly-objectfile gem too? Maybe it's worthy of a new method in the public interface of the gem. (Definitely backloggable if so.)

Copy link
Contributor

@ndushay ndushay left a comment

Choose a reason for hiding this comment

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

wow - this was a huge oversight.

@ndushay
Copy link
Contributor

ndushay commented Sep 18, 2023

@justinlittman do you think we should consider extracting this to the assembly-objectfile gem too? Maybe it's worthy of a new method in the public interface of the gem. (Definitely backloggable if so.)

the main advantage would be computing the two flavors of checksum at the same time if both were desired: https://github.com/sul-dlss/assembly-objectfile/blob/main/lib/assembly/object_file.rb#L89-L98

It might also be worth figuring out if checksums are actually being computed with the gem. https://github.com/sul-dlss/assembly-objectfile/network/dependents

@mjgiarlo
Copy link
Member

@ndushay 💬

It might also be worth figuring out if checksums are actually being computed with the gem. sul-dlss/assembly-objectfile (dependents)

Good call. it could be the spot in common-accessioning that was already optimized for speed using inline code is the only spot where we care about such an optimization, so the extraction could be a chunk of work without much payoff.

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

Successfully merging this pull request may close these issues.

4 participants