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.

Sign up for GitHub

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

Jump to bottom

hammer: mon: drop pg temps from not the current primary in OSDMonitor #9893

Merged
merged 1 commit into from Jul 25, 2016
Merged

hammer: mon: drop pg temps from not the current primary in OSDMonitor #9893

merged 1 commit into from Jul 25, 2016

Conversation

Vicente-Cheng
Copy link
Contributor

@ghost ghost added bug-fix core labels Jun 23, 2016
@ghost ghost added this to the hammer milestone Jun 23, 2016
@ghost ghost self-assigned this Jun 23, 2016
@ghost
Copy link

ghost commented Jun 23, 2016

does this compile if you just replace nullptr with NULL ?

@Vicente-Cheng
Copy link
Contributor Author

@dachary
hi loicd,
I use static_cast to simulate nullptr, it seems OK on my machine.

@ghost
Copy link

ghost commented Jun 24, 2016

make check does not have a problem with it but I don't think it exercises this specific part of the code

@Vicente-Cheng
Copy link
Contributor Author

@dachary
hi loicd, I will still check other reference about how to handle nullptr.
Could I new a vector<int> to null like vector() , and pass it?
please see the latest commit.
how do you think?

@Vicente-Cheng Vicente-Cheng changed the title hammer: OSDMonitor: drop pg temps from not the current primary DNM: hammer: OSDMonitor: drop pg temps from not the current primary Jun 24, 2016
@smithfarm
Copy link
Contributor

smithfarm commented Jul 16, 2016 •
edited

I'm not sure we need to use new here -> that creates a structure in memory (right?), which the original code doesn't do. In hammer we could just use "0" or "NULL" in place of nullptr.

@Vicente-Cheng
Copy link
Contributor Author

@smithfarm
hi Nathan,
OK! I following your suggestion and re-push it.

Actually, I still confuse with NULL and nullptr :(
we could just use value(NULL) to replace the pointer(nullptr) ?

thanks!!

@athanatos @Vicente-Cheng
OSDMonitor: drop pg temp from sources other than the current primary
474abb8 
Fixes: http://tracker.ceph.com/issues/16127
Signed-off-by: Samuel Just <sjust@redhat.com>
(cherry picked from commit 1a07123)

	change:
            use NULL to replace the nullptr because we don't have C++11
@Vicente-Cheng Vicente-Cheng changed the title DNM: hammer: OSDMonitor: drop pg temps from not the current primary hammer: OSDMonitor: drop pg temps from not the current primary Jul 18, 2016
@smithfarm
Copy link
Contributor

@Vicente-Cheng My understanding is that NULL is used in C, and 0 is used in C++. See https://en.wikipedia.org/wiki/Null_pointer#cite_note-4

@smithfarm
Copy link
Contributor

Note that the "null pointer" does not actually point to anything.

@Vicente-Cheng
Copy link
Contributor Author

@smithfarm
OK! Nathan,
thanks for your information : )

that I remove the DNM tag

smithfarm added a commit that referenced this pull request Jul 18, 2016
@smithfarm
Merge pull request #9893: hammer: OSDMonitor: drop pg temps from not …
93540c7 
…the current primary

Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Jul 24, 2016
@smithfarm
Merge pull request #9893: hammer: OSDMonitor: drop pg temps from not …
d3793aa 
…the current primary

Reviewed-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
Copy link
Contributor

@athanatos This PR is in the latest round of hammer-backports integration tests, which passed a rados run (the only failures are a valgrind false positive that has since been fixed by ceph/teuthology#915 and http://tracker.ceph.com/issues/15139 which is an infrastructure issue with two of the tests) - for details, see: http://tracker.ceph.com/issues/15895#note-18

OK to merge?

@athanatos
Copy link
Contributor

lgtm

@smithfarm smithfarm merged commit 67b7f11 into ceph:hammer Jul 25, 2016
@Vicente-Cheng Vicente-Cheng deleted the wip-16430-hammer branch July 26, 2016 02:32
@smithfarm smithfarm changed the title hammer: OSDMonitor: drop pg temps from not the current primary hammer: mon: drop pg temps from not the current primary in OSDMonitor Aug 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug-fix core
Projects
None yet
Milestone
hammer
3 participants
@Vicente-Cheng @smithfarm @athanatos