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.

Sign up for GitHub

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

Jump to bottom

add crc checking feature to upload object #31

Merged
merged 2 commits into from
Oct 21, 2016
Merged

add crc checking feature to upload object #31

merged 2 commits into from
Oct 21, 2016

Conversation

mars-coder
Copy link
Contributor

  1. 在上传文件的操作中(put_object, append_object, upload_part)追加crc校验处理
  2. 由于第三方库性能问题，所以直接采用c扩展库的方式来实现crc计算(ext/crcx)
  3. crc校验根据配置项crc_enable来控制开启
  4. 由于crc校验涉及stream部分处理，上层使用的地方比较多，所以功能测试中分别针对crc enable和crc disable执行了两次
  5. 在Gemfile中限定了第三方库term-ansicolor的版本小于1.4.0，现在默认安装会更新到这个版本，而这个版本要求ruby >= 2.0，和ruby 1.9不兼容

@CLAassistant
Copy link

CLAassistant commented Oct 12, 2016 •
edited

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.007%) to 97.927% when pulling 024706c on features/crc_check into 1df17f7 on master.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 12, 2016 •
edited

Coverage Status

Coverage decreased (-0.007%) to 97.927% when pulling 024706c on features/crc_check into 1df17f7 on master.

@coveralls
Copy link

coveralls commented Oct 13, 2016 •
edited

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling 75720cc on features/crc_check into 1df17f7 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling 75720cc on features/crc_check into 1df17f7 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling 75720cc on features/crc_check into 1df17f7 on master.

rockuw
rockuw reviewed Oct 13, 2016
View reviewed changes
Gemfile
@@ -3,3 +3,6 @@ source 'https://rubygems.org'
gemspec

gem 'coveralls', require: false

# term-ansicolor 1.4.0 requires ruby version >= 2.0, which is incompatible with 1.9.x
gem 'term-ansicolor', '< 1.4.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

我们为什么需要这个依赖？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我们会间接依赖到term-ansicolor，间接依赖中是使用最新版本，目前term-ansicolor新的1.4.0版本没法兼容ruby 1.9，就会导致我们的sdk没法支持ruby 1.9了，所以暂时加了一个版本限定。

rockuw
rockuw reviewed Oct 13, 2016
View reviewed changes
lib/aliyun/oss/util.rb
@@ -86,4 +104,4 @@ def to_bool
return true if self =~ /^true$/i
false
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

这种能在行尾增加一个换行吗？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个没太注意，可能是编辑器自动保存添加的

rockuw
rockuw reviewed Oct 13, 2016
View reviewed changes
spec/aliyun/oss/http_spec.rb
sr << content
end

r = s.read(content.size + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

这里这个+1是什么意思？

@rockuw
Copy link
Contributor

rockuw commented Oct 13, 2016

  1. multipart上传的场景中，没有把各个part的crc最后combine起来，与complete multipart后的整个文件的crc做对比吧？
  2. 由于增加了native extension，在windows环境下测试一下

@coveralls
Copy link

coveralls commented Oct 14, 2016 •
edited

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling b79543d on features/crc_check into 1df17f7 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling b79543d on features/crc_check into 1df17f7 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling b79543d on features/crc_check into 1df17f7 on master.

@mars-coder
add crc checking feature to upload object
62d6fba 
Change-Id: Ibeb03573c76f42ea434f6c51c5f95befc0a5df71
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling 62d6fba on features/crc_check into 1df17f7 on master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling 62d6fba on features/crc_check into 1df17f7 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling 62d6fba on features/crc_check into 1df17f7 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.002% when pulling 62d6fba on features/crc_check into 1df17f7 on master.

@rockuw @mars-coder
Remove pthread for Windows compatibility
5d8a21d 
Change-Id: I8c95b05ff450711552507375199fb6c8294d73e3
@rockuw rockuw merged commit 81c9bc0 into master Oct 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rockuw rockuw rockuw left review comments

Assignees

@rockuw rockuw

@baiyubin baiyubin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mars-coder @CLAassistant @coveralls @rockuw @baiyubin