rados: implement binding for read_op_omap_get_vals_by_keys by gman0 · Pull Request #629 · ceph/go-ceph

gman0 · 2022-01-26T16:31:55Z

This PR implements binding for read_op_omap_get_vals_by_keys RADOS Read operation.

Checklist

Added tests for features and functional changes
Public functions and types are documented
Standard formatting is applied to Go code
Is this a new API? Is this new API marked PREVIEW?

gman0 · 2022-01-26T19:11:30Z

rados/read_op_omap_get_vals_by_keys.go

+// Next gets the next omap key/value pair referenced by
+// ReadOpOmapGetValsByKeysStep's internal iterator.
+// If there are no more elements to retrieve, (nil, nil) is returned.
+// May be called only after Operate() finished.
+//  PREVIEW
+func (s *ReadOpOmapGetValsByKeysStep) Next() (*OmapKeyValue, error) {
+	if !s.canIterate {
+		return nil, ErrOperationIncomplete
+	}
+
+	var (
+		cKey    *C.char
+		cVal    *C.char
+		cValLen C.size_t
+	)
+
+	ret := C.rados_omap_get_next(s.iter, &cKey, &cVal, &cValLen)
+	if ret != 0 {
+		return nil, getError(ret)
+	}
+
+	if cKey == nil {
+		// Iterator has reached the end of the list.
+		return nil, nil
+	}
+
+	return &OmapKeyValue{
+		Key:   C.GoString(cKey),
+		Value: C.GoBytes(unsafe.Pointer(cVal), C.int(cValLen)),
+	}, nil
+}


There are a couple more place in rados's codebase that deals with OMap iterators and this adds an unnecessary amount of boilerplate. Maybe we could factor it out into its own struct and reuse that (probably as a separate PR)?

Actually couple means just ReadOp.GetOmapValues.

Sure, refactoring is generally welcome especially if it reduces the maintenance burden. Usually the only cases where I'd object to it would be cases where copy-n-paste reduces the maintenace burden or if its not really clear the code paths are the same,etc.

For this case I see two main options:

private helper functions that perform much of the boilerplate work

a private embedded struct type that can provide the common methods

We can do the work in a follow up PR or here in this PR, but please let us know what you are thinking so I know how deeply to review the current state in the short term. :-)

@phlogistonjohn thanks for the suggestions. I think doing it in a follow up PR would be cleaner, so this one should be ok to review as is.

phlogistonjohn

LGTM, thanks!

Pull request has been modified.

gman0 · 2022-02-03T13:53:18Z

PTAL @ansiwen when you have some time, I have a few more rados bindings PRs open (3 + this one to be exact) that I hope to get merged for 0.14.0 too.

ansiwen

LGTM, I only have a non-critical remark, I want @gman0 to see before merging. If you are fine as is, let us know, and we will remove the do-not-merge label.

rados/read_op_omap_get_vals_by_keys.go

Pull request has been modified.

rados/read_op_omap_get_vals_by_keys.go

ansiwen · 2022-02-07T17:26:57Z

Sorry, the issue in #630 also applies here. I hope it's ok, but since we will release one week later, I hope you are fine with that late request.

phlogistonjohn · 2022-02-07T22:38:26Z

Sorry, I forgot to use mergify to rebase this and check the CI status... regardless I hope the ci is now fixed

mergify · 2022-02-07T23:08:58Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

ansiwen · 2022-02-07T17:55:36Z

rados/read_op_omap_get_vals_by_keys.go

+		cValLen C.size_t
+	)
+
+	ret := C.rados_omap_get_next(s.iter, &cKey, &cVal, &cValLen)


Another remark, because you will push again anyway: we should probably use rados_omap_get_next2() here, which is more flexible about the keys (can contain 0-bytes).

@ansiwen yeah I was thinking about which version of the function to use here, especially when rados_read_op_omap_get_keys() and rados_read_op_omap_get_vals() are tagged with __attribute__((deprecated)). However WriteOp.SetOmap() uses rados_write_op_omap_set() that expects the keys to be null-terminated:

go-ceph/rados/write_op.go

Lines 93 to 103 in 17fbb4b

// SetOmap appends the map `pairs` to the omap `oid`.

func (w *WriteOp) SetOmap(pairs map[string][]byte) {

sos := newSetOmapStep(pairs)

w.steps = append(w.steps, sos)

C.rados_write_op_omap_set(

w.op,

(**C.char)(sos.cKeys.Ptr()),

(**C.char)(sos.cValues.Ptr()),

(*C.size_t)(sos.cLengths.Ptr()),

sos.cNum)

}

This is also true for rados_read_op_omap_get_vals_by_keys() used in this PR, so I'm not sure using rados_omap_get_next2() alone would make much difference.

So unless we use rados_read_op_omap_get_vals_by_keys2() as well, or better yet, use the *2() versions for all omap ops, it's ok as it is. What do you think?

As you like. You are welcome to change to the *2 versions in a follow up commit. 😉

I'd rather do it separately

Doing it separately is A-OK with me. I highly encourage you to do it soon because I don't love the idea of adding new code based on deprecated functions (assuming I am reading this discussion correctly). I am not asking that you do it before the release, just do it before we all forget. :-)

mergify · 2022-02-08T14:30:19Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

rados/read_op_omap_get_vals_by_keys.go

gman0 · 2022-02-08T15:05:40Z

^ I just tried to resolve the conflicts manually here on github...

gman0 · 2022-02-08T15:43:54Z

@Mergifyio rebase

mergify · 2022-02-08T15:44:20Z

rebase

❌ Base branch update has failed

Details

Git reported the following error:

Rebasing (1/1)
Auto-merging docs/api-status.json
CONFLICT (content): Merge conflict in docs/api-status.json
Auto-merging docs/api-status.md
CONFLICT (content): Merge conflict in docs/api-status.md
error: could not apply 201d1db... rados: implement binding for read_op_omap_get_vals_by_keys
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 201d1db... rados: implement binding for read_op_omap_get_vals_by_keys

err-code: 9C5DE

rados/read_op_omap_get_vals_by_keys.go

mergify · 2022-02-08T19:36:33Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

This commit implements binding for read_op_omap_get_vals_by_keys RADOS Read operation. Includes a unit test. Signed-off-by: Robert Vasek <robert.vasek@cern.ch>

ansiwen

LGTM 🥳 Thanks for bearing with us!

gman0 · 2022-02-09T11:05:05Z

@phlogistonjohn can you please take a look? I believe all the comments so far have been addressed.

Also, when do you roughly expect for 0.14.0 to come out? I don't have any more PRs for the time being, I'm just wondering whether to pin my go.mod dependencies to a commit, or just wait for the release if it's soon enough. Thanks!

phlogistonjohn · 2022-02-09T15:16:41Z

@phlogistonjohn can you please take a look? I believe all the comments so far have been addressed.

I will do so as soon as I can

Also, when do you roughly expect for 0.14.0 to come out? I don't have any more PRs for the time being, I'm just wondering whether to pin my go.mod dependencies to a commit, or just wait for the release if it's soon enough. Thanks!

The next release is scheduled for 2022-02-15. In general, you should be able to use the project milestones to determine when the next release(s) are going to be. I do try to keep them up to date. If it's ever missing, out of date, or confusing, feel free to ping me to get me to fix them.

phlogistonjohn · 2022-02-09T15:24:23Z

rados/read_op_omap_get_vals_by_keys.go

+		cValLen C.size_t
+	)
+
+	ret := C.rados_omap_get_next(s.iter, &cKey, &cVal, &cValLen)


Doing it separately is A-OK with me. I highly encourage you to do it soon because I don't love the idea of adding new code based on deprecated functions (assuming I am reading this discussion correctly). I am not asking that you do it before the release, just do it before we all forget. :-)

gman0 · 2022-02-09T15:57:22Z

Thank you both for great reviews and for getting through those PRs rather fast @phlogistonjohn @ansiwen 👍 ! @phlogistonjohn thanks for the info!

I highly encourage you to do it soon because I don't love the idea of adding new code based on deprecated functions

AFAIK the only deprecations of concern are rados_read_op_omap_get_keys() and rados_read_op_omap_get_vals() -- neither rados_omap_get_next() nor rados_read_op_omap_get_vals_by_keys() used in this PR are tagged as deprecated at the moment (all links are pinned to current master HEAD, btw) -- this could change in the future though?

As for the deprecated ...get_keys() and ...get_vals(): neither of these are in use in go-ceph, only the "v2" of get_vals:

go-ceph/rados/read_op.go

Lines 67 to 84 in 573598c

    
           // GetOmapValues is used to iterate over a set, or sub-set, of omap keys 
        
           // as part of a read operation. An GetOmapStep is returned from this 
        
           // function. The GetOmapStep may be used to iterate over the key-value 
        
           // pairs after the Operate call has been performed. 
        
           func (r *ReadOp) GetOmapValues(startAfter, filterPrefix string, maxReturn uint64) *GetOmapStep { 
        
           	gos := newGetOmapStep(startAfter, filterPrefix, maxReturn) 
        
           	r.steps = append(r.steps, gos) 
        
           	C.rados_read_op_omap_get_vals2( 
        
           		r.op, 
        
           		gos.cStartAfter, 
        
           		gos.cFilterPrefix, 
        
           		C.uint64_t(gos.maxReturn), 
        
           		&gos.iter, 
        
           		gos.more, 
        
           		gos.rval, 
        
           	) 
        
           	return gos 
        
           }

... this is just for peace of mind that there's no actual deprecated code in use. In any case I'll send a PR to "upgrade" them all today or tomorrow.

phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Jan 26, 2022

gman0 commented Jan 26, 2022

View reviewed changes

phlogistonjohn previously approved these changes Jan 30, 2022

View reviewed changes

phlogistonjohn requested a review from ansiwen January 30, 2022 18:43

phlogistonjohn previously approved these changes Feb 4, 2022

View reviewed changes

ansiwen added the do-not-merge label Feb 7, 2022

ansiwen previously approved these changes Feb 7, 2022

View reviewed changes

rados/read_op_omap_get_vals_by_keys.go Outdated Show resolved Hide resolved

gman0 force-pushed the rados_read_op_omap_get_vals_by_keys branch from a1803d7 to c9ed3fa Compare February 7, 2022 15:05

gman0 requested review from ansiwen and phlogistonjohn February 7, 2022 15:36

ansiwen requested changes Feb 7, 2022

View reviewed changes

rados/read_op_omap_get_vals_by_keys.go Show resolved Hide resolved

ansiwen removed the do-not-merge label Feb 7, 2022

ansiwen reviewed Feb 7, 2022

View reviewed changes

gman0 force-pushed the rados_read_op_omap_get_vals_by_keys branch from f6952fb to 201d1db Compare February 8, 2022 14:27

gman0 commented Feb 8, 2022

View reviewed changes

rados/read_op_omap_get_vals_by_keys.go Show resolved Hide resolved

ansiwen reviewed Feb 8, 2022

View reviewed changes

rados/read_op_omap_get_vals_by_keys.go Outdated Show resolved Hide resolved

gman0 force-pushed the rados_read_op_omap_get_vals_by_keys branch 2 times, most recently from 5191df5 to 0cb61e6 Compare February 8, 2022 19:26

rados: implement binding for read_op_omap_get_vals_by_keys

a2b5629

This commit implements binding for read_op_omap_get_vals_by_keys RADOS Read operation. Includes a unit test. Signed-off-by: Robert Vasek <robert.vasek@cern.ch>

gman0 force-pushed the rados_read_op_omap_get_vals_by_keys branch from 0cb61e6 to a2b5629 Compare February 8, 2022 19:45

ansiwen approved these changes Feb 8, 2022

View reviewed changes

phlogistonjohn approved these changes Feb 9, 2022

View reviewed changes

mergify bot merged commit 573598c into ceph:master Feb 9, 2022

gman0 mentioned this pull request Feb 10, 2022

rados: use omap v2 bindings #643

Merged

4 tasks

	// SetOmap appends the map `pairs` to the omap `oid`.
	func (w *WriteOp) SetOmap(pairs map[string][]byte) {
	sos := newSetOmapStep(pairs)
	w.steps = append(w.steps, sos)
	C.rados_write_op_omap_set(
	w.op,
	(**C.char)(sos.cKeys.Ptr()),
	(**C.char)(sos.cValues.Ptr()),
	(*C.size_t)(sos.cLengths.Ptr()),
	sos.cNum)
	}

Conversation

gman0 commented Jan 26, 2022

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn left a comment

Choose a reason for hiding this comment

Uh oh!

gman0 commented Feb 3, 2022

Uh oh!

ansiwen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ansiwen commented Feb 7, 2022

Uh oh!

phlogistonjohn commented Feb 7, 2022

Uh oh!

mergify bot commented Feb 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Feb 8, 2022

Uh oh!

Uh oh!

gman0 commented Feb 8, 2022

Uh oh!

gman0 commented Feb 8, 2022

Uh oh!

mergify bot commented Feb 8, 2022

❌ Base branch update has failed

Uh oh!

Uh oh!

mergify bot commented Feb 8, 2022

Uh oh!

ansiwen left a comment

Choose a reason for hiding this comment

Uh oh!

gman0 commented Feb 9, 2022

Uh oh!

phlogistonjohn commented Feb 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gman0 commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gman0 commented Feb 9, 2022 •

edited

Loading