mon/OSDMonitor: Respect paxos_propose_interval #22243
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
liewegas
approved these changes
May 25, 2018
This change doesn't looks right and causing twice as much proposal as we targeted to (limited by paxos_propose_interval). Imaging we have a sequence of pg_temp/up_thru during a large recovery. now =T The 1st up_thru/pg_temp will go through fast path and trigger propose at T + paxos_min_wait, last_attempted_minwait_time = T. now = T+ paxos_min_wait The [2, K] up_thru will failed by (now - last_attempted_minwait_time > g_conf->paxos_propose_interval) and go through PaxosService::should_propose, which will schedule the propose at) T+paxos_propose_interval now= T+ paxos_propose_interval + paxos_min_wait The K+1 up_thru/pg_temp comes, both (now - last_attempted_minwait_time > g_conf->paxos_propose_interval and now - paxos->get_last_commit_time() > g_conf->paxos_min_wait satisfied, so we trigger another propose in now+ paxos_min_wait = T+ paxos_propose_interval +paxos_min_wait. clearly we made TWO proposal in each paxos_propose_interval. This reverts commit ca65210. Signed-off-by: Xiaoxi CHEN <xiaoxchen@ebay.com>
xiaoxichen
force-pushed
the
fix_osdmap_propose
branch
from
6083cad
to
44283e5
Compare
gregsfortytwo
requested changes
May 25, 2018
| dout(15) << " propose as fast as possible for up_thru/pg_temp" << dendl; | ||
|
|
||
| utime_t now = ceph_clock_now(); | ||
| if (now - last_attempted_minwait_time > g_conf->paxos_propose_interval |
There was a problem hiding this comment.
If there's an issue with the logic, I think it's here — perhaps we need to take the most recent of last_attempted_minwait_time and paxos->get_last_commit_time() in this first check of the if-block?
My head's a little fuzzy right now.
There was a problem hiding this comment.
yes exactly it correctly letting us do one fast .the problem is how to define first commit, the code logic now make it first whenever after a regular commit, so it goes to fast/slow/fast/slow loop instead of fast/slow/slow/slow.
And yes we can fix the logical with adding more if and make it more fuzzy. but it looks to me like the paxoservice::should_propose already implement what we want and clear enough.
utime_t now = ceph_clock_now();
if ((now - paxos->last_commit_time) > g_conf->paxos_propose_interval)
delay = (double)g_conf->paxos_min_wait;
else
delay = (double)(g_conf->paxos_propose_interval + paxos->last_commit_time
- now);
gregsfortytwo
approved these changes
May 25, 2018
liewegas
changed the title
|
can you prepare a backport for mimic and/or luminous? |
liewegas
added a commit
that referenced
this pull request
May 26, 2018
* refs/pull/22243/head: Revert "mon: no delay for single message MSG_ALIVE and MSG_PGTEMP" Reviewed-by: Gregory Farnum <gfarnum@redhat.com> Reviewed-by: Sage Weil <sage@redhat.com>
|
sure, will do it |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change doesn't looks right and causing twice as much proposal as we targeted to (limited by paxos_propose_interval).
Imaging we have a sequence of pg_temp/up_thru during a large recovery.
now =T
The 1st up_thru/pg_temp will go through fast path and trigger propose at T + paxos_min_wait, last_attempted_minwait_time = T.
now = T+ paxos_min_wait
The [2, K] up_thru will failed by (now - last_attempted_minwait_time > g_conf->paxos_propose_interval)
and go through PaxosService::should_propose, which will schedule the propose at) T+paxos_propose_interval
now= T+ paxos_propose_interval + paxos_min_wait
The K+1 up_thru/pg_temp comes, both
(now - last_attempted_minwait_time > g_conf->paxos_propose_interval)
and
(now - paxos->get_last_commit_time() > g_conf->paxos_min_wait)
satisfied, so we trigger another propose at now+ paxos_min_wait = T+ paxos_propose_interval +paxos_min_wait.
clearly we made TWO proposal in each paxos_propose_interval.
This reverts commit ca65210.
Signed-off-by: Xiaoxi CHEN xiaoxchen@ebay.com