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

[MRG+1] Make FilesPipeline work with S3FilesStore using botocore #1883

Merged
merged 2 commits into from Mar 31, 2016

Conversation

@lopuhin
Copy link
Member

@lopuhin lopuhin commented Mar 27, 2016

It did not work due to two reasons, one silly and one intricate:

  • meta=None was not handled properly
  • there was a race condition between persisting file and calculating checksum (explained in the commit message) - this in theory could lead to bugs with other stores as well.
lopuhin added 2 commits Mar 25, 2016
Checksum calculation could happen simultaniously with
persisting the file in the store (which is done in a thread):
they operated on the same buf object.
Concretely this lead to a bug with S3FilesStore
when using botocore: the signature did not match because
the position in the buf was already at the end.
The fix is to move checksum calculation before passing buf
to the store.
@codecov-io
Copy link

@codecov-io codecov-io commented Mar 27, 2016

Current coverage is 83.18%

Merging #1883 into master will not affect coverage as of 48aa230

Powered by Codecov. Updated on successful CI builds.

@kmike kmike changed the title Make FilesPipeline work with S3FilesStore using botocore [MRG+1] Make FilesPipeline work with S3FilesStore using botocore Mar 27, 2016
@kmike kmike added this to the v1.1 milestone Mar 27, 2016
@redapple redapple merged commit 9ae4e46 into scrapy:master Mar 31, 2016
2 checks passed
2 checks passed
codecov/patch 100.00% of diff hit (target 100.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lopuhin lopuhin deleted the lopuhin:botocore-files-store-fix branch Apr 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants