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

doc: op queue and mclock related options #16662

Merged
merged 1 commit into from Aug 5, 2017

Conversation

Projects
None yet
5 participants
@ivancich
Member

ivancich commented Jul 28, 2017

Additionally makes sure when unexpected values are provided for config
options osd_op_queue or osd_op_queue_cut_off that the default values
are assigned.

Signed-off-by: J. Eric Ivancich ivancich@redhat.com

@ivancich ivancich requested review from tchaikov and jdurgin Jul 28, 2017

Option("osd_op_queue_cut_off", Option::TYPE_STR, Option::LEVEL_ADVANCED)
.set_default("low")
.set_description(""),
.set_description("the threshold between high priority ops that use strict priority ordering and low priority ops that use a fairness algorithm that may or may not incorporate prioirty; can be one of three values -- 'low', 'high', and 'debug_random'")

This comment has been minimized.

@Yan-waller

Yan-waller Jul 29, 2017

Contributor

how about using .set_long_description() here and adding some brief description to .set_description()?
others ditto.

This comment has been minimized.

@ivancich

ivancich Jul 31, 2017

Member

So I did that. I looked through options.h to see if there was any guidance about how long a description (not long description) would ideally be and couldn't find anything. Any thoughts?

This comment has been minimized.

@Yan-waller

Yan-waller Jul 31, 2017

Contributor

I'm not very sure, but from my point, set_descriptione() is used to describe what it is, while set_long_description() is used to describe somethings more details, such as the optional parameters and how to use it, etc..

@@ -1881,71 +1881,315 @@ std::vector<Option> global_options = {
Option("osd_op_queue", Option::TYPE_STR, Option::LEVEL_ADVANCED)
.set_default("wpq")
.set_description(""),
.set_description("which operation queue algorithm to use; can be one of five values -- 'wpq', 'prioritized', 'mclock_opclass', 'mclock_client', and 'debug_random'")

This comment has been minimized.

@tchaikov

tchaikov Jul 31, 2017

Contributor

how about using set_enum_allowed() to enforce the constraint?

This comment has been minimized.

@ivancich

ivancich Jul 31, 2017

Member

Good idea. Done.

@ivancich

This comment has been minimized.

Member

ivancich commented Jul 31, 2017

jenkins retest this please

.set_description(""),
.set_enum_allowed( { "low", "high", "debug_random" } )
.set_description("the threshold between high priority ops and low priority ops")
.set_long_description("the threshold between high priority ops that use strict priority ordering and low priority ops that use a fairness algorithm that may or may not incorporate prioirty")

This comment has been minimized.

@jdurgin

jdurgin Aug 1, 2017

Member

typo in the last 'priority'

.set_description(""),
.set_enum_allowed( { "wpq", "prioritized", "mclock_opclass", "mclock_client", "debug_random" } )
.set_description("which operation queue algorithm to use")
.set_description("which operation queue algorithm to use; mclock_opclass and mclock_client are currently experimental")

This comment has been minimized.

@tchaikov

tchaikov Aug 1, 2017

Contributor

this should be set_long_description(), i guess? and i am not sure if it helps to repeat the short version of description in the long description.

This comment has been minimized.

@ivancich

ivancich Aug 1, 2017

Member

I'd like to see some overall guidance so this documentation is consistent throughout ceph. Max length for description. Whether long description should contain description.

This comment has been minimized.

@tchaikov

tchaikov Aug 1, 2017

Contributor

@ivancich does this help?

@tchaikov tchaikov added the needs-qa label Aug 1, 2017

@tchaikov tchaikov added this to the luminous milestone Aug 1, 2017

@ivancich

This comment has been minimized.

Member

ivancich commented Aug 2, 2017

I just rebased it on master to get it past merge conflicts.

.set_enum_allowed( { "wpq", "prioritized", "mclock_opclass", "mclock_client", "debug_random" } )
.set_description("which operation queue algorithm to use")
.set_long_description("which operation queue algorithm to use; mclock_opclass and mclock_client are currently experimental")
.add_service("osd")

This comment has been minimized.

@liewegas

liewegas Aug 2, 2017

Member

these add_service() lines aren't needed now if it's in teh osd section

doc: op queue and mclock related options
Additionally makes sure when unexpected values are provided for config
options osd_op_queue or osd_op_queue_cut_off that the default values
are assigned.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>

@liewegas liewegas merged commit dbb4c1c into ceph:master Aug 5, 2017

3 of 4 checks passed

make check (arm64) make check failed
Details
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment