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

osd: Cleanup-Updated OSDMap.cc with C++11 style range-for loops #14381

Merged
merged 4 commits into from Apr 18, 2017

Conversation

Projects
None yet
4 participants
@joscollin
Member

joscollin commented Apr 7, 2017

Updated OSDMap.cc with C++11 style range-for loops in template iterations for better
readability. Also replaced some iterator types with auto, which stores the return value of find().

Signed-off-by: Jos Collin jcollin@redhat.com

osd: Updated OSDMap.cc with C++11 style range-for loops
Updated OSDMap.cc with C++11 style range-for loops in template iterations for better
readability. Also replaced some iterator types with auto, which stores the return value of find().

Signed-off-by: Jos Collin <jcollin@redhat.com>
@tchaikov

these loop variables deserve a better name.

++q) {
map<int64_t,pg_pool_t>::iterator r = new_pools.find(*q);
for (auto &elemm : new_pools) {

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

could use a better name than elemm, maybe new_pool?

if (!elemm.second.tiers.empty()) {
pg_pool_t& base = elemm.second;
for (const auto &elems : base.tiers) {

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

s/elems/tier_pool/

@@ -247,8 +240,8 @@ bool OSDMap::subtree_is_down(int id, set<int> *down_cache) const
list<int> children;
crush->get_children(id, &children);
for (list<int>::iterator p = children.begin(); p != children.end(); ++p) {
if (!subtree_is_down(*p, down_cache)) {
for (const auto &elem : children) {

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

s/elem/child/

p != new_pools.end();
++p) {
n = p->first;
for (const auto &elem : new_pools) {

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

s/elem/new_pool/

for (map<int64_t, string>::const_iterator p = new_pool_names.begin(); p != new_pool_names.end(); ++p) {
n = p->first;
for (const auto &elem : new_pool_names) {

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

s/elem/new_pool_name/

}
// for ::encode(old_pools, bl);
n = old_pools.size();
::encode(n, bl);
for (set<int64_t>::iterator p = old_pools.begin(); p != old_pools.end(); ++p) {
n = *p;
for (auto &elem : old_pools) {

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

s/elem/old_pool/

++p) {
old_pg_t opg = p->first.get_old_pg();
for (const auto &elem : new_pg_temp) {

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

s/elem/pg_temp/

@@ -743,82 +734,86 @@ void OSDMap::Incremental::dump(Formatter *f) const
f->dump_int("new_max_osd", new_max_osd);
f->open_array_section("new_pools");
for (map<int64_t,pg_pool_t>::const_iterator p = new_pools.begin(); p != new_pools.end(); ++p) {
for (const auto &elem : new_pools) {

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

s/elem/new_pool/

f->close_section();
}
f->close_section();
f->open_array_section("new_pool_names");
for (map<int64_t,string>::const_iterator p = new_pool_names.begin(); p != new_pool_names.end(); ++p) {
for (const auto &elem : new_pool_names) {

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

s/new_pool_names/new_pool_name/

for (set<int64_t>::const_iterator p = old_pools.begin(); p != old_pools.end(); ++p)
f->dump_int("pool", *p);
for (const auto &elem : old_pools)

This comment has been minimized.

@tchaikov

tchaikov Apr 8, 2017

Contributor

s/elem/old_pool/

This comment has been minimized.

@joscollin

joscollin Apr 9, 2017

Member

@tchaikov Fixed as per the review comments. Thanks for the suggestions.

osd: Updated OSDMap.cc with C++11 style range-for loops
Incorporated review comments by adding meaningful names for the loop variables.

Signed-off-by: Jos Collin <jcollin@redhat.com>
@tchaikov

tchaikov requested changes Apr 9, 2017 edited

i am trying to enumerate all "elem" your change, but maybe you can identify the rest of them without my help?

elem is good for a name of variable in books for data structure or algorithm libraries, but not for a variable with some tangible meaning.

f->open_array_section("osds");
for (const auto &elems : elemp.second)
for (const auto &elems : elempg.second)

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elems/osd/

@@ -2510,11 +2510,11 @@ void OSDMap::dump(Formatter *f) const
}
f->close_section();
f->open_array_section("pg_temp");
for (const auto &elemp : *pg_temp) {
for (const auto &elempg : *pg_temp) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elempg/pg/

f->open_array_section("osds");
for (const auto &elemv : elemm.second)
f->dump_int("osd", elemv);
for (const auto &elems : pg_temp.second)

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elems/osd/

f->open_array_section("state_xor");
for (auto &elems : st)
f->dump_string("state", elems);
for (auto &elemst : st)

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elemst/state/

@@ -2486,15 +2431,15 @@ void OSDMap::dump(Formatter *f) const
f->dump_int("max_osd", get_max_osd());
f->open_array_section("pools");
for (map<int64_t,pg_pool_t>::const_iterator p = pools.begin(); p != pools.end(); ++p) {
for (const auto &elem : pools) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elem/pool/

++p) {
n = p->first;
for (const auto &elem : pools) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elem/pool/

for (vector<int>::iterator p = osds.begin(); p != osds.end(); ++p) {
if (!exists(*p))
*p = CRUSH_ITEM_NONE;
for (auto &elem : osds) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elem/osd/ and no need to reference it by reference.

p != inc.old_blacklist.end();
++p)
blacklist.erase(*p);
for (const auto &elem : inc.old_blacklist)

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elem/addr

++i) {
int s = i->second ? i->second : CEPH_OSD_UP;
if ((osd_state[i->first] & CEPH_OSD_UP) &&
for (const auto &elem : inc.new_state) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elem/state/ or better off define an const local variable to reference elem.first, like

const auto osd = state.first;
i != inc.old_erasure_code_profiles.end();
++i)
erasure_code_profiles.erase(*i);
for (const auto &elem : inc.old_erasure_code_profiles)

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

s/elem/profile/

for (vector<string>::iterator i = sections.begin(); i != sections.end(); ++i) {
if (i->find("osd.") != 0)
for (auto &elem : sections) {

This comment has been minimized.

@joscollin

joscollin Apr 9, 2017

Member

@tchaikov Why there are two sections vectors here ? It is confusing. Can I change the second one to 'newsections' ? Will it break the logic ?

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

could you point me to the other sections?

This comment has been minimized.

@joscollin

joscollin Apr 9, 2017

Member

There are two sections at 3015 and 3029. There is no logical error it seems. But for a reader, it looks confusing.

// add osds
  vector<string> sections;
  conf->get_all_sections(sections);

  for (auto &elem : sections) {
    if (elem.find("osd.") != 0)
      continue;

    const char *begin = elem.c_str() + 4;
    char *end = (char*)begin;
    int o = strtol(begin, &end, 10);
    if (*end != '\0')
      continue;

    string host, rack, row, room, dc, pool;
    vector<string> sections;
    sections.push_back("osd");
sections.push_back(elem);

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

maybe we can take care of this in another PR?

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

maybe we can improve this in another PR.

osd: Updated OSDMap.cc with C++11 style range-for loops
Fixed the review comments by replacing all the 'elem' variables with different meaningful names.

Signed-off-by: Jos Collin <jcollin@redhat.com>
f->close_section();
}
f->close_section();
f->open_array_section("new_blacklist");
for (const auto &elem : new_blacklist) {
for (const auto &blacklist : new_blacklist) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

the element in a blacklist is not blacklist, they are addresses and their expire times.

This comment has been minimized.

@joscollin

joscollin Apr 9, 2017

Member

The new_blacklist is declared as:
map<entity_addr_t,utime_t> new_blacklist;
If so, blacklist.first would be the address and blacklist.second would be the expire times. This is similar for old_blacklist too. I should have discuss this before I made the previous modifications to the *blacklist templates. Please share your thoughts and clarify. Thanks.

for (const auto &elem : *primary_temp) {
f->dump_stream("pgid") << elem.first;
f->dump_int("osd", elem.second);
for (const auto pg : *primary_temp) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

const auto&

This comment has been minimized.

@joscollin

joscollin Apr 10, 2017

Member

@liewegas My intention was to improve the Ceph code to the newer standard and better readable code. But the following is the comment from Kefu, which is also a valid point. So I request you to provide your suggestions on this, so that we could proceed with either merge or close this PR. Thanks.

Your change render it more difficult for us to identify when a change is introduced and why. fwiw, we use "git blame" to identify what a commit changes, or which commit a certain piece of code belongs. If we have lots of massive cleanup like this, the commit history is more difficult to read for maintainers. But seems Sage is okay with it and that's why i tried to improve your PR before it gets merged. But i don't like it personally, it makes the code easier to read, but more difficult to maintain.

@@ -2510,29 +2515,29 @@ void OSDMap::dump(Formatter *f) const
}
f->close_section();
f->open_array_section("pg_temp");
for (const auto &elempg : *pg_temp) {
for (const auto pg : *pg_temp) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

const auto&

@@ -2463,8 +2468,8 @@ void OSDMap::dump(Formatter *f) const
set<string> st;
get_state(i, st);
f->open_array_section("state");
for (auto &elem : st)
f->dump_string("state", elem);
for (auto &state : st)

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

const auto&

}
}
for (auto &elem : *tmpmap.primary_temp) {
for (auto pg : *tmpmap.primary_temp) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

auto&

p != new_weight.end();
++p) {
if (p->second == CEPH_OSD_OUT && !previous->is_out(p->first))
for (const auto &weight : new_weight) {

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

we can dispense const here. as this is a const method.

}
f->close_section();
f->open_array_section("old_blacklist");
for (vector<entity_addr_t>::const_iterator p = old_blacklist.begin(); p != old_blacklist.end(); ++p)
f->dump_stream("addr") << *p;
for (const auto &blacklist : old_blacklist)

This comment has been minimized.

@tchaikov

tchaikov Apr 9, 2017

Contributor

const auto &addr

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 10, 2017

osd: Updated OSDMap.cc with C++11 style range-for loops
Fixed the review comments.

Signed-off-by: Jos Collin <jcollin@redhat.com>
@joscollin

This comment has been minimized.

Member

joscollin commented Apr 11, 2017

@liewegas Thank you for the suggestion.
@tchaikov Thanks for the review and approval.

Test this please.

@yuriw yuriw merged commit b0ea1f1 into ceph:master Apr 18, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@joscollin joscollin deleted the joscollin:wip-cleanup-osdmap-rangefor branch Apr 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment