Skip to content
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: break sending data-log list infinitely #16926

Merged

Conversation

fangyuxiangGL
Copy link
Contributor

No description provided.

@fangyuxiangGL
Copy link
Contributor Author

fangyuxiangGL commented Aug 9, 2017

Hi @cbodley

I saw a phenomenon that radosgw send data log list op to the opposite end infinitely, which cause the opposite end stuck with high cpu usage.
http://tracker.ceph.com/issues/20951

@fangyuxiangGL fangyuxiangGL force-pushed the infinitely-send-data-log-list branch 2 times, most recently from 97c61dd to 923985b Compare August 9, 2017 04:28
@fangyuxiangGL
Copy link
Contributor Author

Jenkins retest this please

Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just commenting for now; orit?

@@ -1042,6 +1042,8 @@ class RGWDataSyncShardCR : public RGWCoroutine {
RGWDataChangesLogInfo shard_info;
string datalog_marker;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need line spaces, I don't think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattbenjamin thanks, addressed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, this works great

#define INCREMENTAL_MAX_ENTRIES 100
ldout(sync_env->cct, 20) << __func__ << ":" << __LINE__ << ": shard_id=" << shard_id << " datalog_marker=" << datalog_marker << " sync_marker.marker=" << sync_marker.marker << dendl;
if (datalog_marker > sync_marker.marker) {
spawned_keys.clear();
if (sync_marker.marker.empty())
remote_trimmed = 2; //remote data log shard might be trimmed;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the 0..1 values of remote_trimmed actually ordinals? if not, I think they should be constants or enumeration values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mattbenjamin

Not ordinals, I just used 2 to flag "might be trimmed", and 1 to flag "was trimmed", and 0 to flag "not trimmed"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case, there should probably something like
enum class TrimState : uint8_t { NOT_TRIMMED, TRIMMED, MAYBE_TRIMMED } ;
or your preferred way of expressing this in c++

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(iif you want to keep the test for "remote trimmed" as written, you could do it static constexpr of the values as you wrote them rather than an enum)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattbenjamin

thanks, did you suggest that I use macro?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fangyuxiangGL sorry, I'd like to see us use some kind of compile-time constants--I was trying to say, it doesn't have to be an enum or enum class (in particular, enum class won't allow you to use the two non-zero vaues as a boolean without a case -I think-), but if you don't object, you could use that

@@ -1042,6 +1042,7 @@ class RGWDataSyncShardCR : public RGWCoroutine {
RGWDataChangesLogInfo shard_info;
string datalog_marker;

int remote_trimmed;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ in my comments about using a symbolic constant, I meant to hold the values assigned to remote_trimmed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok , addressed it

@fangyuxiangGL
Copy link
Contributor Author

@mattbenjamin

Hi, changed it to enum type. thanks!

@fangyuxiangGL
Copy link
Contributor Author

ping @mattbenjamin

radosgw send data-log list infinitely when opposite end trimmed the data-log and in quiescence.
Fixes: http://tracker.ceph.com/issues/20951

Signed-off-by: fang yuxiang fang.yuxiang@eisoo.com
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this addresses my concern

@@ -1042,6 +1042,8 @@ class RGWDataSyncShardCR : public RGWCoroutine {
RGWDataChangesLogInfo shard_info;
string datalog_marker;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, this works great

@cbodley
Copy link
Contributor

cbodley commented Aug 22, 2017

@mattbenjamin if you're satisfied with changes, could you please update your review?

@yuriw
Copy link
Contributor

yuriw commented Aug 23, 2017

@mattbenjamin mattbenjamin merged commit 25ffe9c into ceph:master Aug 23, 2017
@fangyuxiangGL fangyuxiangGL deleted the infinitely-send-data-log-list branch January 10, 2018 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants