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

osd/OSDMap: improve the performance of pg_to_acting_osds #12190

Merged
merged 1 commit into from Dec 12, 2016
Merged

osd/OSDMap: improve the performance of pg_to_acting_osds #12190

merged 1 commit into from Dec 12, 2016

Conversation

liupan1111
Copy link
Contributor

…up vector" is not requested.

Signed-off-by: Pan Liu pan.liu@istuary.com

@liupan1111 liupan1111 closed this Nov 26, 2016
@liupan1111 liupan1111 reopened this Nov 28, 2016
@liupan1111
Copy link
Contributor Author

The case is: when pg temp is not empty, acting will be calculated without ups and up_primary. But ups and up_primary are always calculated, and affect the performance at this situation.

@liupan1111
Copy link
Contributor Author

@liewegas @tchaikov , please help take a look.

@liupan1111
Copy link
Contributor Author

I find one thing very weird. I created this pr yesterday morning, and found one testcase failed when ran jekins:
qa/workunits/cephtool/test.sh : test_mon_crushmap_validation
"Error EINVAL: Parse error setting osd_scrub_auto_repair_num_errors to '-1' using injectargs."

So I had to close this pr, and investigated what was wrong, but nothing found. When I reopened this pr without any changes, jekins passed....

Anyone could give me any idea?

@liewegas liewegas changed the title OSDMap: improve the performance of pg_to_acting_osds. In which case "… osd/OSDMap: improve the performance of pg_to_acting_osds Nov 28, 2016
@liewegas
Copy link
Member

@liupan1111 there are still some issues with the unit tests; it sounds like it wasn't caused by this change.

_raw_to_up_osds(*pool, raw, &_up, &_up_primary);
_apply_primary_affinity(pps, *pool, &_up, &_up_primary);
if (_acting.empty()) {
_acting = _up;
Copy link
Member

Choose a reason for hiding this comment

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

indentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

@liupan1111 ping?

Copy link
Contributor

Choose a reason for hiding this comment

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

and could you fix the Signed-off-by line in your commit message? or just squash these two commits into a single one?

_acting = _up;
if (_acting_primary == -1) {
_acting_primary = _up_primary;
}
}
}
if (up)
Copy link
Member

Choose a reason for hiding this comment

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

you could mvoe the if (up) and if (up_prmiary) lines into the block?

Copy link
Member

@liewegas liewegas left a comment

Choose a reason for hiding this comment

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

a cosmetic changes, otherwise lgtm

@liupan1111
Copy link
Contributor Author

@liewegas , modified as your request.

_raw_to_up_osds(*pool, raw, &_up, &_up_primary);
_apply_primary_affinity(pps, *pool, &_up, &_up_primary);
if (_acting.empty()) {
_acting = _up;
Copy link
Contributor

Choose a reason for hiding this comment

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

@liupan1111 could you fix the indent?

@liupan1111
Copy link
Contributor Author

@tchaikov done, thanks!

@tchaikov
Copy link
Contributor

@liupan1111 could you fix the signed-off-by line in 993d37d, or better off squashing these two commit into a single one?

@liupan1111
Copy link
Contributor Author

@tchaikov what is wrong with signed-off-by?

@tchaikov
Copy link
Contributor

- Signed-off-by: Pan Liu pan.liu@istuary.com
+ Signed-off-by: Pan Liu <pan.liu@istuary.com>

Signed-off-by: Pan Liu <pan.liu@istuary.com>
@liupan1111
Copy link
Contributor Author

@tchaikov modified.

@tchaikov tchaikov self-assigned this Nov 29, 2016
@tchaikov tchaikov merged commit 2bdf593 into ceph:master Dec 12, 2016
@tchaikov
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants