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, osd: misc fixes #16078

Merged
merged 6 commits into from Jul 7, 2017

Conversation

Projects
None yet
5 participants
@xiexingguo
Member

xiexingguo commented Jul 3, 2017

No description provided.

@liewegas liewegas added the needs-qa label Jul 3, 2017

@xiexingguo

This comment has been minimized.

Member

xiexingguo commented Jul 3, 2017

retest this please

cct->get_admin_socket()->unregister_command("dump_historic_slow_ops");
cct->get_admin_socket()->unregister_command("dump_historic_ops_by_duration");

This comment has been minimized.

@tchaikov

tchaikov Jul 4, 2017

Contributor

would be better to oder the unregister calls in lexicographical order.

This comment has been minimized.

@xiexingguo

xiexingguo Jul 4, 2017

Member

I am trying to rearrange them by the order they are registered. So it'll be convenient to find out those singletons next time.
I guess performance is your main concern? That will be less signicant as we are going to shutdown anyway...

This comment has been minimized.

@tchaikov

tchaikov Jul 4, 2017

Contributor

not here.

This comment has been minimized.

@xiexingguo

xiexingguo Jul 4, 2017

Member

do you mean changing all cmds in lexicographical order?

This comment has been minimized.

@xiexingguo

xiexingguo Jul 4, 2017

Member

including both register and unregister process?

This comment has been minimized.

@Liuchang0812

Liuchang0812 Jul 4, 2017

Contributor

I already unregister those admin socket commands in #16045. and there are some indentation problem in register commands code.

This comment has been minimized.

@tchaikov

tchaikov Jul 4, 2017

Contributor

@xiexingguo i was just looking at the "dump_historic_ops_by_duration" line. because your change is increasing the inversion number.

This comment has been minimized.

@xiexingguo

xiexingguo Jul 4, 2017

Member

yeah, and I think I have answered your question:



> I am trying to rearrange them by the order they are registered. So it'll be convenient to find out those singletons next time.
> I guess performance is your main concern? That will be less signicant as we are going to shutdown anyway...

This comment has been minimized.

@tchaikov

tchaikov Jul 4, 2017

Contributor

i don't think i am asking any questions here? and i am trying to clarify my intention by answering yours:

including both register and unregister process?

This comment has been minimized.

@xiexingguo

xiexingguo Jul 4, 2017

Member

Sigh.

Exclude this one since @Liuchang0812 has done it.

@@ -3996,6 +3996,8 @@ bool OSDMonitor::preprocess_command(MonOpRequestRef op)
rdata.append(ds);
}
release_p:

This comment has been minimized.

@tchaikov

tchaikov Jul 4, 2017

Contributor

better off using a scope_guard here.

@@ -100,10 +100,13 @@ COMMAND("osd test-reweight-by-pg " \
COMMAND("osd scrub " \
"name=who,type=CephString", \
"initiate scrub on osd <who>", "osd", "rw", "cli,rest")
"initiate scrub on osd <who>, or use <all|any|*> to scrub all", \

This comment has been minimized.

@tchaikov

tchaikov Jul 4, 2017

Contributor

why shall we add "all|any"? is this a convention we need to follow? if yes, could you point me any of the places where we are following it?

This comment has been minimized.

@xiexingguo

xiexingguo Jul 4, 2017

Member

as per discussion with sage at #15381

It would be nice to move this into a helper, though, so that the other commands that take lists of osds can also inherit the 'any' syntax (and maybe we can also make 'all' or '*' work too?).

could you point me any of the places where we are following it?

osd downosd out、etc.

This comment has been minimized.

@tchaikov

tchaikov Jul 4, 2017

Contributor

fair enough. although i am not sure the use case of "all|any".

xiexingguo added some commits Jul 3, 2017

osd/OSD: update check_full_status() to accept a pre-calculated ratio …
…only

We don't have to pass in the osd_stat, which is not necessary.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
osd/OSD: refactor update_osd_stat() to narrow lock
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
mon/OSDMonitor: introduce osd_pool_default_type config option
In luminous we now have full support of ec pool, so it is not
good to continue hardcoding default pool type to "replicated".

Introduce a configurable osd_pool_default_type for this.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@@ -3883,6 +3883,12 @@ bool OSDMonitor::preprocess_command(MonOpRequestRef op)
p->decode(osdmap_bl);
}
BOOST_SCOPE_EXIT((p) (&osdmap)) {

This comment has been minimized.

@tchaikov

tchaikov Jul 4, 2017

Contributor

please use the scope_guard, which is simpler.

delete p;
}
};
scope_guard<decltype(cleanup)> sg(std::move(cleanup));

This comment has been minimized.

@tchaikov

tchaikov Jul 6, 2017

Contributor

would be simpler if make_scope_guard is used.

@@ -3996,8 +4004,7 @@ bool OSDMonitor::preprocess_command(MonOpRequestRef op)
rdata.append(ds);
}
if (p != &osdmap)
delete p;

This comment has been minimized.

@tchaikov

tchaikov Jul 6, 2017

Contributor

please drop the empty line.

xiexingguo added some commits Jul 3, 2017

mon/OSDMonitor: fix potential memory leak
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
mon/OSDMonitor: add more wildcards support for scrub and fix description
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
mon/OSDMonitor: tidy up upmap on pool removal
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo

This comment has been minimized.

Member

xiexingguo commented Jul 6, 2017

Repushed

@yuriw yuriw merged commit 795eccf into ceph:master Jul 7, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@xiexingguo xiexingguo deleted the xiexingguo:wip-misc branch Jul 22, 2017

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