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

rgw: add a separate configuration for data notify interval #16551

Merged
merged 1 commit into from Aug 7, 2017

Conversation

Projects
None yet
3 participants
@fangyuxiangGL
Contributor

fangyuxiangGL commented Jul 25, 2017

use rgw_md_notify_interval_msec to control both meta and data notify notify seems not reasonable.
Generally speaking, I prefer to use longer data notify interval than meta, which increase probability that meta arrive before data sync, so data sync won't trigger RGWMetaSyncSingleEntryCR for no meta exists in local.

Maybe quantify it is varying, but we should have a separate configuration firstly.

@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented Jul 25, 2017

@cbodley

please have a look, thanks

@@ -1659,6 +1659,7 @@ OPTION(rgw_log_http_headers, OPT_STR, "" ) // list of HTTP headers to log when s
OPTION(rgw_num_async_rados_threads, OPT_INT, 32) // num of threads to use for async rados operations
OPTION(rgw_md_notify_interval_msec, OPT_INT, 200) // metadata changes notification interval to followers
OPTION(rgw_data_notify_interval_msec, OPT_INT, 200) // data changes notification interval to followers

This comment has been minimized.

@cbodley

cbodley Jul 25, 2017

Contributor

could you please add this to the new common/options.cc also? see the new entry for rgw_md_notify_interval_msec

This comment has been minimized.

@fangyuxiangGL

fangyuxiangGL Jul 26, 2017

Contributor

@cbodley , done it

@cbodley cbodley added the rgw label Jul 25, 2017

@@ -3115,7 +3115,7 @@ class RGWDataNotifier : public RGWRadosThread {
RGWDataNotifierManager notify_mgr;
uint64_t interval_msec() override {
return cct->_conf->rgw_md_notify_interval_msec;
return cct->_conf->rgw_data_notify_interval_msec;

This comment has been minimized.

@cbodley

cbodley Jul 26, 2017

Contributor

if this doesn't exist in legacy_config_opts.h, we need to access it by string instead. something like this?

  return cct->_conf->get_val<int>("rgw_data_notify_interval_msec");

This comment has been minimized.

@fangyuxiangGL

fangyuxiangGL Jul 26, 2017

Contributor

thanks, seems that config options changed a lot. i have changed it.

This comment has been minimized.

@cbodley

cbodley Jul 26, 2017

Contributor

yeah, John Spray described the changes on the ceph-devel list, see https://www.spinics.net/lists/ceph-devel/msg37602.html

This comment has been minimized.

@fangyuxiangGL

fangyuxiangGL Jul 26, 2017

Contributor

I don't know why i have missed the mail, will read it carefully.

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jul 26, 2017

there's a conflict in options.cc, could you please rebase?

@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented Jul 26, 2017

thanks, it's ok now.

@cbodley cbodley added the needs-qa label Jul 26, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jul 26, 2017

jenkins test this please

@@ -3115,7 +3115,7 @@ class RGWDataNotifier : public RGWRadosThread {
RGWDataNotifierManager notify_mgr;
uint64_t interval_msec() override {
return cct->_conf->rgw_md_notify_interval_msec;
return cct->_conf->get_val<int>("rgw_data_notify_interval_msec");

This comment has been minimized.

@cbodley

cbodley Aug 1, 2017

Contributor

getting a FAILED assert("wrong type or option does not exist" == nullptr) from this line

This comment has been minimized.

@cbodley

cbodley Aug 1, 2017

Contributor

it looks like that needs to be get_val<int64_t>, because the md_config_t::member_ptr_t variant doesn't have an int

This comment has been minimized.

@fangyuxiangGL

fangyuxiangGL Aug 1, 2017

Contributor

@cbodley , already change it to int64_t, but frankly speaking, I am not sure that the issue is addressed.

IMHO, the traditional style is better, at least complain will be reported in compile time, not running time.

@yuriw

This comment has been minimized.

@cbodley

This comment has been minimized.

Contributor

cbodley commented Aug 2, 2017

rebase again please?

rgw: add a separate configuration for data notifier interval
Signed-off-by: fang yuxiang <fang.yuxiang@eisoo.com>
@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented Aug 3, 2017

@cbodley addressed

@yuriw yuriw merged commit 59a0f3c into ceph:master Aug 7, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@fangyuxiangGL fangyuxiangGL deleted the fangyuxiangGL:data-notifier-interval branch Jan 10, 2018

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