-
Notifications
You must be signed in to change notification settings - Fork 124
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
5794 download file redux #5987
5794 download file redux #5987
Conversation
file_metadata = find_file_metadata(file_set: file_set, use: use) | ||
file = Hyrax.storage_adapter.find_by(id: file_metadata.file_identifier) | ||
prepare_file_headers_valkyrie(metadata: file_metadata, file: file) | ||
file.rewind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does appear to load the file in to memory: https://share.getcloudapp.com/Blub19om
I used this code to evaluate before and after the rewind
module MemInfo
# This uses backticks to figure out the pagesize, but only once
# when loading this module.
# You might want to move this into some kind of initializer
# that is loaded when your app starts and not when autoload
# loads this module.
KERNEL_PAGE_SIZE = `getconf PAGESIZE`.chomp.to_i rescue 4096
STATM_PATH = "/proc/#{Process.pid}/statm"
STATM_FOUND = File.exist?(STATM_PATH)
def self.rss
STATM_FOUND ? (File.read(STATM_PATH).split(' ')[1].to_i * KERNEL_PAGE_SIZE) / 1024 : 0
end
end
just a note. specs are working in pg now, but failing in dassie / non-valkerie. need to figure out how to have all three. |
@orangewolf is this something it would be helpful for me to carry over the line today/tomorrow? |
@no-reply for sure - I'm sorry I missed your earlier comment. I'm out on vacation. I haven't had time to figure out why the specs are mad. |
3d0342d
to
ad378b4
Compare
…in file downloads
rfc9110 14.2 says: > A server MUST ignore a Range header field received with a request method that > is unrecognized or for which range handling is not defined. For this specification, > GET is the only method for which range handling is defined.
i think the remaining issue here is that the test setup on the valkyrie specs isn't valid for AF/Wings. an easy fix for this could be to simply not use this controller behavior when Wings is in use, and skip the tests under those conditions. i think in that case we're missing test coverage for Valkyrie altogether, though, since |
@no-reply i agree with you. There isn’t really a Valkyrie pg test set up. Do you or @dlpierce have to circleci knowledge to easily create one? If so, I’m tempted to say we do so and then I can task a junior to mark the failures pending if and only if they are use_Valkyrie and not wings. That lets us get green and move in to the new Valkyrie work (which starts issue review and testing on Monday) with a functioning test suite we can add to. |
@orangewolf I'll propose getting Circle setup for this testing as a topic for this week's dev con that I'll lead. |
…specifies Ruby version, we should not also specify in the Gemfile. Only listing it once makes it easier to update correctly and also means changing the Dockerfile ARG can actually build the app successfully.
surfliner version of downloads_controller_spec.rb |
These remaining spec failures where describedby is nil seem to point to the AF::File/Hyrax::FileMetadata conversion being incomplete. The spec for persisting a file is a stub. I expect this is due to the special handling for some File metadata, as mention in the commit. I suggest making sure the Hyrax::FileSet factory can correctly use its |
I spent a lot of time trying to get the specs to persist files properly. I got it almost working, but then realized that it slowed the specs way down. So instead we'll use the test adapters for this spec. That keeps it speedy and keeps it focused on testing the Valkyrie controller logic and not on testing the underlying library. |
@dlpierce this is down to just non-related flaky specs. are you ok with the proposed testing solution? |
…cted them to be. this was due to Valkyrie using a different pairtree method than Hyrax
…o-find-than-wifes-headphones thumbnails and other derivatives were not stored in the path we expected them to be
Fixes #5794 ; Depends on merge of #5626
Adopts surfliner's DownloadsController and uses it when using valkyrie. Allows download of files stored in a valkyrie storage adapter.