-
Notifications
You must be signed in to change notification settings - Fork 6k
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/MDSMonitor: use stringstream instead of dout for mds repaired #28683
Conversation
src/mon/MDSMonitor.cc
Outdated
} else { | ||
dout(1) << "repaired: no-op on rank " << role << dendl; | ||
ss << "repaired: no-op on rank " << role; |
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.
If both this is essentially a no-op, it's basically the same as saying it was modified from the user's point of view. I don't think it's useful to return to the user the fact that it is a no-op, even if it's useful to log it.
I'd just push the return message to ss
regardless of the branch, and log each branch's message to dout nonetheless.
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.
If both this is essentially a no-op, it's basically the same as saying it was modified from the user's point of view. I don't think it's useful to return to the user the fact that it is a no-op, even if it's useful to log it.
These messages across the MDSMonitor tend to err on the side of overly helpful (or absent 😦). I think it's okay to give helpful information (that the rank is not damaged).
With that said, "no-op" is not the right message, let's use:
ss << "nothing to do: rank is not damaged";
I'd just push the return message to ss regardless of the branch, and log each branch's message to dout nonetheless.
We don't do that for any other message so I don't see a reason to start. These commands are already logged.
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.
Yeah, that's fine with me. The 'no-op' was the bit that really seemed off; especially because no-ops in the monitor tend to be assumed as an idempotent operation of whatever we're trying to achieve. If not being damaged is the information we want actually want to relay, then 👍
Fixes: http://tracker.ceph.com/issues/40472 Signed-off-by: Zhi Zhang <zhangz.david@outlook.com>
@batrick Pls review the latest changes, thanks. |
@batrick shall we run this through a cephfs qa suite? |
Sure. Will do! |
* refs/pull/28683/head: mon/MDSMonitor: use stringstream instead of dout for mds repaired Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>
Fixes: http://tracker.ceph.com/issues/40472
Signed-off-by: Zhi Zhang zhangz.david@outlook.com