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/MgrMonitor: send digests only if is_active() #15109

Merged
merged 4 commits into from May 18, 2017

Conversation

Projects
None yet
3 participants
@tchaikov
Contributor

tchaikov commented May 16, 2017

labeling this a bug fix, because we should cancel an event before resetting it, otherwise the event is leaked.

@tchaikov tchaikov requested review from liewegas and jcsp May 16, 2017

@tchaikov tchaikov changed the title from mon/MgrMonitor: send digest in tick() to mon/MgrMonitor: send digests in tick() May 16, 2017

@@ -346,6 +337,12 @@ void MgrMonitor::tick()
return;
const auto now = ceph::coarse_mono_clock::now();
if (is_active() &&
chrono::duration_cast<chrono::seconds>(now - last_sent_digest).count() >
g_conf->mon_mgr_digest_period) {

This comment has been minimized.

@jcsp

jcsp May 16, 2017

Contributor

Hmm, this is going to give pretty irregular messages if mon_mgr_digest_period is not a multiple of the tick period.

@jcsp

This comment has been minimized.

Contributor

jcsp commented May 16, 2017

I'm sort of unsure how much simpler this is. If digests were always sent at the tick frequency then it would be obviously simpler, but when we have a different frequency for digests vs tick, it was clearer to explicitly use timer events than it is to try and get the digest frequency using a conditional in tick

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 16, 2017

@jcsp makes sense! fixed and repushed.

@tchaikov tchaikov changed the title from mon/MgrMonitor: send digests in tick() to mon/MgrMonitor: send digests only if is_active() May 16, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented May 16, 2017

In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/mon/Monitor.cc:77:0:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/mon/MgrMonitor.h: In constructor ‘MgrMonitor::MgrMonitor(Monitor*, Paxos*, const string&)’:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/mon/MgrMonitor.h:46:42: error: class ‘MgrMonitor’ does not have any field named ‘digest_callback’
     : PaxosService(mn, p, service_name), digest_callback(nullptr)
@liewegas

lgtm except for build issue

tchaikov added some commits May 16, 2017

mon/MgrMonitor: s/digest_callback/digest_event/
to be consistent with the rest of mon

Signed-off-by: Kefu Chai <kchai@redhat.com>
mon/MgrMonitor: cancel timer before resetting it
Signed-off-by: Kefu Chai <kchai@redhat.com>
mon/MgrMonitor: use mono clock to avoid potention issue introduced by…
… time skew

Signed-off-by: Kefu Chai <kchai@redhat.com>
mon/MgrMonitor: send digests only if is_active()
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 16, 2017

@liewegas fixed and pushed.

@tchaikov tchaikov added the needs-qa label May 17, 2017

@liewegas liewegas merged commit 5d83781 into ceph:master May 18, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@tchaikov tchaikov deleted the tchaikov:wip-mgrmon-send-digests-using-ticks branch May 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment