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

remove magic number #900

Merged
merged 1 commit into from
Jun 20, 2017
Merged

remove magic number #900

merged 1 commit into from
Jun 20, 2017

Conversation

taotaowill
Copy link
Contributor

@CLAassistant
Copy link

CLAassistant commented Jun 15, 2017 •
edited
Loading

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@lpstudy
Copy link

lpstudy commented Jun 16, 2017

有些问题： FLAGS_chunkserver_disk_safe_space << 20， 而原代码是5<<30， 也就是说单位是GB，现在变成了<<20， 也就是MB，这是一个问题。 还有FLAGS_chunkserver_disk_safe_space并没有任何单位的标识，那其他人看到这个变量如何得知它的单位呢？ 建议使用： FLAGS_chunkserver_disk_safe_space_GB 这样的词汇更能见名知意。

@yvxiang
Copy link
Collaborator

yvxiang commented Jun 16, 2017

@lpstudy flags.cc这个文件里说明了这个变量的单位

yvxiang
yvxiang reviewed Jun 16, 2017
View reviewed changes
src/nameserver/chunkserver_manager.cc Outdated
@@ -335,7 +336,8 @@ double ChunkServerManager::GetChunkServerLoad(ChunkServerInfo* cs) {
double data_score = cs->data_size() * 1.0 / cs->disk_quota();
int64_t space_left = cs->disk_quota() - cs->data_size();

if (data_score > 0.95 || space_left < (5L << 30) || pending_score > 1.0) {
if (data_score > 0.95 || pending_score > 1.0
|| space_left < (FLAGS_chunkserver_disk_safe_space << 20)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

flags.cc里chunkserver_disk_safe_space的默认值为5120

Copy link

Choose a reason for hiding this comment

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

恩恩，我看到了。 flags.cc中确实标记了单位。 不过一眼看到两个单词有错误，哈哈
(line 82, 83 in flags.cc).

  1. the disk will be concidered full. considered
  2. Used to computer disk wordload : workload

Copy link
Collaborator

Choose a reason for hiding this comment

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

估计手抖了，请提个pr改了吧：）

Copy link

Choose a reason for hiding this comment

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

还有如果既然修改了这个，那么data_score > 0.95中的0.95这样的字样似乎也应该修改为FLAG标记。

Copy link
Collaborator

Choose a reason for hiding this comment

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

嗯，可以改成FLAG

lpstudy added a commit to lpstudy/bfs that referenced this pull request Jun 16, 2017
@lpstudy
(baidu#901)
94bacd4 
small fix to avoid the conflict with the pull request of (remove magic number baidu#900)
@bluebore bluebore merged commit d540d18 into baidu:master Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpstudy lpstudy lpstudy left review comments

@yvxiang yvxiang yvxiang approved these changes

@bluebore bluebore bluebore approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@taotaowill @CLAassistant @lpstudy @yvxiang @bluebore