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

osd: PGLog: split divergent priors as well #4631

Merged
merged 1 commit into from Jul 10, 2015

Conversation

Projects
None yet
4 participants
@smithfarm
Contributor

smithfarm commented May 9, 2015

@tchaikov tchaikov added this to the firefly milestone May 9, 2015

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 9, 2015

In file included from ./osd/PG.h:39:0,
                 from ./osd/OSD.h:20,
                 from osd/ClassHandler.cc:4:
./osd/PGLog.h: In member function 'void PGLog::split_into(pg_t, unsigned int, PGLog*)':
./osd/PGLog.h:405:22: error: 'struct hobject_t' has no member named 'get_hash'
       if ((i->second.get_hash() & mask) == child_pgid.m_seed) {
                      ^

might want to backport hobject_t::get_hash() introduced by 6de83d4 to firefly also.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 9, 2015

@tchaikov: thanks!

@ghost ghost changed the title from PGLog: split divergent priors as well to DNM: PGLog: split divergent priors as well May 10, 2015

@ghost ghost self-assigned this May 10, 2015

@ghost

This comment has been minimized.

ghost commented May 10, 2015

@smithfarm I assume you did not cherry-pick -x 6de83d4 because it has conflicts. And that's why you chose to just add the get_hash function. In that case I think it's better to add that as a "Conflict" resolution in the original patch, with a comment explaining what you had to do and why you did it. So the reviewer understands the problem and can approve the solution you chose.

The problem with a minimalist approach is that it may create problems with future backports, if they need the full extent of 6de83d4 instead of just the get_hash backport you did. If that happens it may be necessary to try to backport 6de83d4 in full and resolve the conflict. And it will be necessary to relate to the partial backport you did to explain why part of the patch was already there and not the rest.

There seems to be only two rather limited conflicts when applying 6de83d4 (did not check if it compiles though, maybe that's the real problem). Did you try it ?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 10, 2015

@dachary: Actually I did consider attempting to backport 6de83d4 but was too lazy. Now at your suggestion I made an attempt, but quickly got in over my head as 6de83d4 depends on 4c2828e.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 10, 2015

@dachary: Now trying a new approach in a89983b

@ghost

This comment has been minimized.

ghost commented May 10, 2015

@smithfarm sensible approach, simpler !

@ghost

This comment has been minimized.

ghost commented May 10, 2015

@smithfarm do you mind cherry-pick -x and add a "Conflict" comment to explain your approach ?

PGLog: split divergent priors as well
Signed-off-by: Samuel Just <sjust@redhat.com>
(cherry picked from commit bbe231a)

Conflicts:
    src/osd/PGLog.h

    The cherry-picked commit did not compile as-is, because the hobject_t
    class in firefly lacks a get_hash() method, which was added in 6de83d4.
    To get the patch to compile, I replaced i->second.get_hash() with
    i->second.hash.
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 10, 2015

@dachary: something like this?

@smithfarm smithfarm changed the title from DNM: PGLog: split divergent priors as well to PGLog: split divergent priors as well May 10, 2015

@ghost

This comment has been minimized.

ghost commented May 10, 2015

@smithfarm perfect

@ghost ghost assigned smithfarm and unassigned ghost Jun 2, 2015

athanatos pushed a commit that referenced this pull request Jul 10, 2015

Samuel Just
Merge pull request #4631 from SUSE/wip-11069-firefly
PGLog: split divergent priors as well

Reviewed-by: Samuel Just

@athanatos athanatos merged commit 741f0c2 into ceph:firefly Jul 10, 2015

@smithfarm smithfarm deleted the SUSE:wip-11069-firefly branch Jul 10, 2015

@ghost ghost changed the title from PGLog: split divergent priors as well to osd: PGLog: split divergent priors as well Jul 21, 2015

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