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

mempool: fix lack of pool names in mempool:dump output for JSON forma… #18329

Merged
merged 1 commit into from Nov 1, 2017

Conversation

ifed01
Copy link
Contributor

@ifed01 ifed01 commented Oct 16, 2017

…tter.

Signed-off-by: Igor Fedotov ifed@mail.ru

@ifed01 ifed01 added the bug-fix label Oct 16, 2017
@@ -42,13 +42,16 @@ const char *mempool::get_pool_name(mempool::pool_index_t ix) {
void mempool::dump(ceph::Formatter *f)
{
stats_t total;
f->open_object_section("mempool"); // we need (dummy?) topmost section for
// JSON Formatter to print pool names. It omits them otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

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

@ifed01 i think we could add a dummy section here, like

f->open_object_section("mempool");
f->open_array_section("pools");
for (...) {
  ...
}
f->close_section();
f->dump_object("total", total);
f->close_section();

as the pools are of the same type (pool_t), while total is a stats_t. what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. will do

Copy link
Contributor Author

@ifed01 ifed01 Oct 17, 2017

Choose a reason for hiding this comment

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

@tchaikov - unfortunately it looks like top level blocks inside an array can't have names as well - hence we either need yet another dummy nested level or should print pool names as a field within a block not as a block name. Consider:

{
"pools" : [
   {
     "pool1" : {
        "a":"1"
      }
   },
   {
     "pool2" : {
       "a":"1"
     }
   }
]
}
vs.
{
"pools" : [
 {
   "name" : "pool2"
   "a":"1"
 },
 {
   "name" : "pool2"
   "a":"1"
 }
]
}

Not sure adding array section worths such changes... Please note that these 'dummy' block names will be visible in other formats hence wasting the output...

Copy link
Contributor

Choose a reason for hiding this comment

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

no, elements in an array does not have their names. so, right, the output would look like the latter.

IMHO, the problem of mempool::dump() are two folded:

  1. no top level data structure. that's what your change is addressing
  2. "total" is not a pool, but it is printed in line with other pools.

…tter.

Signed-off-by: Igor Fedotov <ifed@mail.ru>
@ifed01
Copy link
Contributor Author

ifed01 commented Oct 25, 2017

@tchaikov: made an object section for pools instead of array one to avoid additional nested level. Please take a look.

@ifed01
Copy link
Contributor Author

ifed01 commented Oct 30, 2017

@tchaikov: ping

@tchaikov tchaikov merged commit db89b2c into ceph:master Nov 1, 2017
@ifed01 ifed01 deleted the wip-fix-mempool-dump branch November 1, 2017 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants