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

mon/PGMonitor: fix description for ceph pg ls, etc. #12807

Merged
merged 1 commit into from Jan 17, 2017

Conversation

runsisi
Copy link
Contributor

@runsisi runsisi commented Jan 6, 2017

  1. ceph pg ls-by-pool does not support poolid, so modify the description for it
  2. add osdid as an arg for ceph pg ls, so we can support both:
    ceph pg ls <poolid> <osdid> <states> and
    ceph pg ls <poolid> <states>

Signed-off-by: runsisi runsisi@zte.com.cn

@liuchang0812
Copy link
Contributor

it would be better to squash commits like:

git rebase -i <commit id>

@xiexingguo
Copy link
Member

This looks reasonable to me.

@tchaikov What do you think?

@tchaikov
Copy link
Contributor

tchaikov commented Jan 9, 2017

@xiexingguo will take a look tmr.

@@ -155,6 +155,7 @@ COMMAND("pg ls-by-osd " \
"list pg on osd [osd]", "pg", "r", "cli,rest")
COMMAND("pg ls " \
"name=pool,type=CephInt,req=false " \
"name=osd,type=CephInt,req=false " \
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think "pg ls" command accepts three optional arguments. it does accept two optional argument.

the reason why else if (prefix == "pg ls") { branch checks for the "pool" is just that it is reused by "pg ls-by-pool", and "pg ls-by-pool" sets the "pool" argument programmatically:

    cmd_putval(g_ceph_context, cmdmap, "pool", pool);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i know the trick :) thank you for your detailed explanation!
if ceph command accepts named arguments, maybe we could change it from positional argument to named argument. anyway, i will drop the second commit for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't really think we should expose the implementation details in the help message.

@@ -142,7 +142,7 @@ COMMAND("pg dump_stuck " \
COMMAND("pg ls-by-pool " \
"name=poolstr,type=CephString " \
"name=states,type=CephChoices,strings=active|clean|down|replay|scrubbing|degraded|inconsistent|peering|repair|recovering|backfill_wait|incomplete|stale|remapped|deep_scrub|backfill|backfill_toofull|recovery_wait|undersized|activating|peered,n=N,req=false ", \
"list pg with pool = [poolname | poolid]", "pg", "r", "cli,rest")
"list pg with pool = [poolname]", "pg", "r", "cli,rest")
Copy link
Contributor

Choose a reason for hiding this comment

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

please update the man page accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks!

@tchaikov tchaikov assigned runsisi and unassigned tchaikov Jan 12, 2017
@tchaikov
Copy link
Contributor

@runsisi ping?

@runsisi
Copy link
Contributor Author

runsisi commented Jan 17, 2017

@liuchang0812 it's two commits for different purposes :)

@runsisi
Copy link
Contributor Author

runsisi commented Jan 17, 2017

@tchaikov sorry for the late response, i was absent from work for a few days :)

@tchaikov tchaikov merged commit d45f1ae into ceph:master Jan 17, 2017
@runsisi runsisi deleted the wip-fix-pg-ls branch January 17, 2017 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants