Wip 16998 #10688
Wip 16998 #10688
Conversation
Signed-off-by: Samuel Just <sjust@redhat.com>
| for (vector<int>::iterator p = up.begin(); p != up.end(); ++p, ++i) { | ||
| pg_shard_t s(*p, | ||
| pool.info.ec_pool() ? shard_id_t(i) : shard_id_t::NO_SHARD); | ||
| for (set<pg_shard_t>::const_iterator p = upset.begin(); |
There was a problem hiding this comment.
what is the difference between this approach and the existing way? i think they are the same.
There was a problem hiding this comment.
can we skip the shards who are CRUSH_ITEM_NONE here instead without introducing this.upset? and could use auto and range based loop.
There was a problem hiding this comment.
It's really meant to match the actingset and actingbackfill sets. Mostly, using the up and acting vectors directly has led to a ton of subtle bugs like this one, and switching to actingset/actingbackfill has helped a lot. Also, having the upset isn't really all that costly.
There was a problem hiding this comment.
also, could use:
for (const auto &s : upset) {
here, and replace *p with s at https://github.com/ceph/ceph/pull/10688/files#diff-dfb9ddca0a3ee32b266623e8fa489626R2562 and https://github.com/ceph/ceph/pull/10688/files#diff-dfb9ddca0a3ee32b266623e8fa489626R2567.
There was a problem hiding this comment.
Eh, I wanted it to be symmetric with the actingset scan above. Also, we might want to backport these to hammer.
|
nit, might want to connect to http://tracker.ceph.com/issues/16998 using "Fixes: http://tracker.ceph.com/issues/16998"? |
Fixes: http://tracker.ceph.com/issues/16998 Signed-off-by: Samuel Just <sjust@redhat.com>
|
Added the Fixes: line (thanks!). |
|
modulo a nit. lgtm. |
|
retest this please. |
|
lgtm. |
|
retest this please. |
No description provided.