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
message: optimazation for message priority strategy #8687
Conversation
cost(0) | ||
{} | ||
cost(0) { | ||
set_priority(CEPH_MSG_PRIO_HIGH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not this one, ReplicatedBackend bases the priority on the Push priority.
PushReply has it's priority already set in ReplicatedBackend based on the Push priority -- that's better than setting it to HIGH unconditionally. Remove the PushReply change. The two OpReply messages already have HIGH set in ReplicatedBackend. It's confusing to set it in both places. If you want to switch it to be set in the constructor, please separate that change into its own commit and remove the set_priority calls from ReplicatedBackend. Setting the peering message priority in the constructor does make some sense, but don't do it in the same commit as the OpReply messages. |
want peering as fast as it can Signed-off-by: yaoning <yaoning@unitedstack.com>
edfb5e7
to
86cda40
Compare
@athanatos , updated, please review again. I think it is not reasonable to set the priority for MOSDSubOpReply since it is also used by others (like scrubbing). So I revert all modification related to Reply Ops, it would be fine and sometime more instructive to guide us setting different priority in different cases. |
@mslovy That second commit should be a separate PR. Currently, there are no tests at all that have different clients at different priorities, I would be surprised if it worked. That is work that needs to be done. The first step would be to add into the ceph-qa-suite rados/thrash suite a workload with two ceph_test_rados instances using different client priorities. Once we have that to test against, we can start merging patches to fix that behavior. |
86cda40
to
ad9786b
Compare
@athanatos , reset the second commit and I will propose a new PR later as mentioned |
Ok, this makes sense, adding the needs-qa label. |
want peering and reply as fast as it can
Signed-off-by: Ning Yao yaoning@unitedstack.com