Merged
Conversation
gman0
force-pushed
the
omap-keys-v2
branch
2 times, most recently
from
3c32e59 to
68745c6
Compare
Collaborator
|
Awesome, thanks! Will review soon. |
ansiwen
added
API
ansiwen
reviewed
Mar 1, 2022
Collaborator
ansiwen
left a comment
ansiwen
left a comment
There was a problem hiding this comment.
I only have one little comment request, otherwise looks good. 👍
gman0
commented
Mar 4, 2022
Contributor
Author
|
Contributor
Author
|
If you are ok with the changes please let me know if the commits need to be squashed. |
Collaborator
No, I'm fine with it as is. Only if there is a back and forth or fix-ups in the commits, I would prefer them to be squashed, but it seems to me, that your commits are not that. |
Contributor
Author
|
@Mergifyio rebase |
BufferGroup is a helper structure that holds Go-allocated slices of C-allocated strings and their respective lengths. Useful for C functions that consume byte buffers with explicit length instead of null-terminated strings.
GetOmapSte.Next() and ReadOpOmapGetValsByKeysStep.Next() now use rados_omap_get_next2().
ReadOp.GetOmapValuesByKeys() now uses rados_read_op_omap_get_vals_by_keys2().
C strings that are passed in to rados_read_op_omap_get_vals2() are copied by the function, see [1][2] for its implementation. It's therefore not necessary for them to be owned by GetOmapStep. It's enough to construct and destruct them in ReadOp.GetOmapValues. [1] https://github.com/ceph/ceph/blob/2bd3dd512ab42f6e5d5d0dd5f5428b783681802e/src/librados/librados_c.cc#L4334-L4358 [2] https://github.com/ceph/ceph/blob/2bd3dd512ab42f6e5d5d0dd5f5428b783681802e/src/include/rados/librados.hpp#L572-L587
WriteOp.SetOmap now uses rados_write_op_omap_set2(). The entirety of setOmapStep struct was removed as it's not necessary for it to own the various C-allocated values passed to rados_write_op_omap_set2() as arguments. rados_write_op_omap_set2() makes copies of all the string-based args [1], so it's sufficient for them to be short lived. [1] https://github.com/ceph/ceph/blob/2bd3dd512ab42f6e5d5d0dd5f5428b783681802e/src/librados/librados_c.cc#L3852-L3871
WriteOp.RmOmapKeys now uses rados_write_op_omap_rm_keys2(). The entirety of removeOmapKeysStep struct was removed as it's not necessary for it to own the various C-allocated values passed to rados_write_op_omap_rm_keys2() as arguments. rados_write_op_omap_rm_keys2() makes copies of all the string-based args [1], so it's sufficient for them to be short lived. [1] https://github.com/ceph/ceph/blob/2bd3dd512ab42f6e5d5d0dd5f5428b783681802e/src/librados/librados_c.cc#L3888-L3902
✅ Branch has been successfully rebased |
Contributor
Author
|
@phlogistonjohn @ansiwen I think all comments have been addressed. Can you take a look please? |
phlogistonjohn
approved these changes
Mar 9, 2022
Collaborator
phlogistonjohn
left a comment
phlogistonjohn
left a comment
There was a problem hiding this comment.
Thanks for all the hard work & documenting the various places the ceph code does copies! It makes it easy for me to be confident in these simplifications.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a follow-up to our previous discussion in #629 (comment) .
All RADOS bindings that handle OMap keys now use their "more flexible" librados versions that consume byte strings with explicit lengths instead of expecting them to be null-terminated.
I have done a bit of refactoring too -- creating new C-allocated arrays to store the key sizes seemed like too much boilerplate. To that end I have added
cutil.GoCSLenSlicethat handles byte arrays and size arrays in one go.Also, it seems that some of the
*Stepstructs were unnecessary: these particular C functions in librados make copies of their args anyway, so it seems there is no need to keep them around for longer in the*Stepstructs...I've tried to split this PR into multiple commits for easier review. Please see commit messages for more details about the individual changes.
Checklist