rados: implement binding for rados_read_op_read #615

gman0 · 2021-12-03T09:24:37Z

This PR adds binding for rados_read_op_read RADOS Read operation including a unit test for it.

It also adds a common readStep struct as it was missing.

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 · 2021-12-03T09:31:41Z

Currently, in order for users to read the data off of the buffer, they need to do buffer[:ReadOpReadStep.BytesRead]. One alternative approach to what I've provided in this PR is that instead of users passing buffer to ReadOp.Read, they would only provide maximum number of bytes to read. ReadOpReadStep struct would then contain Buffer []bytes field that would hold the actual data read from the object. Also, len(Buffer) == BytesRead. In that case we could also get rid of BytesRead, since this information would come from len(ReadOpReadStep.Buffer).

I think this ^ would be a bit more user-friendly, but perhaps further from its C form. I'll let reviewers decide which one is better and will change this PR accordingly.

gman0 · 2021-12-03T09:34:22Z

Please ignore the CI error

# github.com/ceph/go-ceph/rados
rados/read_op_preview.go:47:32: undefined: cutil.IntSize

cutil.IntSize comes from #581. I'll rebase this once the other PR gets in.

phlogistonjohn · 2021-12-03T15:10:45Z

Currently, in order for users to read the data off of the buffer, they need to do buffer[:ReadOpReadStep.BytesRead]. One alternative approach to what I've provided in this PR is that instead of users passing buffer to ReadOp.Read, they would only provide maximum number of bytes to read. ReadOpReadStep struct would then contain Buffer []bytes field that would hold the actual data read from the object. Also, len(Buffer) == BytesRead. In that case we could also get rid of BytesRead, since this information would come from len(ReadOpReadStep.Buffer).

I think this ^ would be a bit more user-friendly, but perhaps further from its C form. I'll let reviewers decide which one is better and will change this PR accordingly.

Thanks for mentioning this. I think it's fine in the current form. A "short read" is handled the same way in idiomatic go when using something like an io.Reader too. So it's not just a closeness to C argument I think. Plus if there was high demand for this proposed form, it'd be relatively easy to implement as a helper building on top of this step, but the other way around would not be possible. :-)

I'll try to look at the full implementation soon, but I'm going to have limited availability for the next few days.

ansiwen

Should we call the file just read_op_raed.go, then we don't have to change the filename, when it's changing to stable.

rados/read_step.go

ansiwen · 2021-12-07T17:54:38Z

rados/read_op_preview.go

+}
+
+// Read bytes from offset into buffer.
+// `len(buffer)` is the maximum number of bytes read from the object.


Go comments don't support this markdown style formatting.

Yup, I've just seen them being used on a couple of places, e.g.

go-ceph/internal/cutil/aliases.go

Lines 36 to 43 in 1d9c3f8

// CharPtrPtr is an unsafe pointer wrapping C's `char**`.

type CharPtrPtr unsafe.Pointer

// CharPtr is an unsafe pointer wrapping C's `char*`.

type CharPtr unsafe.Pointer

// SizeTPtr is an unsafe pointer wrapping C's `size_t*`.

type SizeTPtr unsafe.Pointer

... but then there are other functions right next to them and they are not using backticks. If this causes an eyesore I can remove them for consistency sake.

You are right, these are also "wrong", but they are in an internal package, so no documentation is created from it. Your comments are however part of the external interface and will show up in the docs, so I would prefer to keep it to the Go conventions.

ansiwen · 2021-12-07T18:00:56Z

rados/read_op_preview.go

+//                          size_t * bytes_read,
+//                          int * prval)
+func (r *ReadOp) Read(offset uint64, buffer []byte) *ReadOpReadStep {
+	oe := newReadStep(buffer, uint64(len(buffer)), offset)


If the second argument is always len(buffer), we don't need it and can get len(buffer) in newReadStep().

Well, right now there are no other cases where the length differs. But maybe in the future there might be one, and all the occurrences would have to be refactored? I can't give examples where this would be true from the top of my head though...

The Go idiomatic way would be to use a slice of the buffer then, like buffer[:N], because that doesn't cause a copy. I think it would not be good coding style to give a function the whole buffer, but then tell it: "oh but please don't use the whole buffer but only the beginning of it".

And anyway I hope there will be not many occurrences of a function called newReadStep(). ;-)

gman0 · 2021-12-08T17:04:06Z

Should we call the file just read_op_raed.go, then we don't have to change the filename, when it's changing to stable.

Is that the plan? To have each OP implementation in a separate file? If that's so then sure, that makes more sense -- only the build tag would be removed.

gman0 · 2022-01-05T10:01:41Z

@ansiwen @phlogistonjohn Hello and happy new year! Just a reminder for this PR because I haven't received any responses for my last comments -- unless it means you insist on all of them?

ansiwen · 2022-01-05T10:14:23Z

Should we call the file just read_op_raed.go, then we don't have to change the filename, when it's changing to stable.

Is that the plan? To have each OP implementation in a separate file? If that's so then sure, that makes more sense -- only the build tag would be removed.

There is no real plan, but I think it doesn't hurt, given that we have the new "preview" policy in place.

ansiwen · 2022-01-05T10:18:04Z

@ansiwen @phlogistonjohn Hello and happy new year! Just a reminder for this PR because I haven't received any responses for my last comments -- unless it means you insist on all of them?

Yeah, Thanks for the reminder. I answered now. @phlogistonjohn will be back at work tomorrow.

gman0 · 2022-01-07T15:00:32Z

Hmm, I got an "ok" from running make test-container locally:

PASS
coverage: 93.6% of statements in github.com/ceph/go-ceph/rados
ok  	github.com/ceph/go-ceph/rados	59.720s	coverage: 93.6% of statements in github.com/ceph/go-ceph/rados

@phlogistonjohn @ansiwen can you please take a look? I hope I addressed all of the comments so far.

phlogistonjohn

Nothing in the code jumps out at me and the API looks OK and the tests look OK.
We do need to fix the version numbers in the API metadata tho as we have made additional releases since you originally generated the files. Sorry for the inconvenience.

phlogistonjohn · 2022-01-07T16:33:47Z

docs/api-status.json

+      {
+        "name": "ReadOp.Read",
+        "comment": "Read bytes from offset into buffer.\nlen(buffer) is the maximum number of bytes read from the object.\nbuffer[:ReadOpReadStep.BytesRead] then contains object data.\n PREVIEW\n\nImplements:\n void rados_read_op_read(rados_read_op_t read_op,\n                         uint64_t offset,\n                         size_t len,\n                         char * buffer,\n                         size_t * bytes_read,\n                         int * prval)\n",
+        "added_in_version": "v0.12.0",


The numbers here have to be updated. The next upcoming release is v0.14

phlogistonjohn · 2022-01-07T16:35:14Z

docs/api-status.md

@@ -13,6 +13,7 @@
 Name | Added in Version | Expected Stable Version | 
 ---- | ---------------- | ----------------------- | 
 WriteOp.CmpExt | v0.12.0 | v0.14.0 | 
+ReadOp.Read | v0.12.0 | v0.14.0 | 


Same here. This file is generated based on the json - you can either update this by hand or by running the apiage tool. For a trivial edit the by-hand appraoch is probably faster :-)

rados/read_step.go

This commit implements binding for rados_read_op_read RADOS Read operation. Includes a unit test. Signed-off-by: Robert Vasek <[email protected]>

gman0 · 2022-01-11T13:32:58Z

@ansiwen @phlogistonjohn I've re-run make api-update just to be sure I didn't miss anything. The version docs should be now ok, PTAL.

phlogistonjohn

LGTM thanks!

ansiwen

LGTM, thank you!

gman0 · 2022-01-13T14:28:26Z

Thank you both for feedback and reviews! It seems though this is not getting merged because of the failing "Summary" CI job?

phlogistonjohn · 2022-01-13T14:37:44Z

Thanks for pointing that out. Mergify config appears broken. I'll just merge it manually now and look into the mergify issue later today. Thanks!

gman0 changed the title ~~rados: implement binding for rados_read_op_read~~ [WIP] rados: implement binding for rados_read_op_read Dec 3, 2021

phlogistonjohn added API This PR includes a change to the public API of a go-ceph package do-not-merge labels Dec 3, 2021

ansiwen requested changes Dec 7, 2021

View reviewed changes

gman0 force-pushed the rados_read_op_read branch from b3de1a4 to be5e4cf Compare January 7, 2022 14:06

phlogistonjohn requested changes Jan 7, 2022

View reviewed changes

rados: implement binding for rados_read_op_read

037e5eb

This commit implements binding for rados_read_op_read RADOS Read operation. Includes a unit test. Signed-off-by: Robert Vasek <[email protected]>

gman0 force-pushed the rados_read_op_read branch from be5e4cf to 037e5eb Compare January 11, 2022 12:53

gman0 changed the title ~~[WIP] rados: implement binding for rados_read_op_read~~ rados: implement binding for rados_read_op_read Jan 11, 2022

phlogistonjohn approved these changes Jan 11, 2022

View reviewed changes

ansiwen approved these changes Jan 13, 2022

View reviewed changes

phlogistonjohn merged commit 937f8d8 into ceph:master Jan 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rados: implement binding for rados_read_op_read #615

rados: implement binding for rados_read_op_read #615

gman0 commented Dec 3, 2021

gman0 commented Dec 3, 2021

gman0 commented Dec 3, 2021 •

edited

Loading

phlogistonjohn commented Dec 3, 2021

ansiwen left a comment

ansiwen Dec 7, 2021

gman0 Dec 8, 2021

ansiwen Jan 5, 2022

ansiwen Dec 7, 2021

gman0 Dec 8, 2021

ansiwen Jan 5, 2022

gman0 commented Dec 8, 2021

gman0 commented Jan 5, 2022 •

edited by ansiwen

Loading

ansiwen commented Jan 5, 2022

ansiwen commented Jan 5, 2022

gman0 commented Jan 7, 2022

phlogistonjohn left a comment

phlogistonjohn Jan 7, 2022

phlogistonjohn Jan 7, 2022

gman0 commented Jan 11, 2022

phlogistonjohn left a comment

ansiwen left a comment

gman0 commented Jan 13, 2022

phlogistonjohn commented Jan 13, 2022

	// CharPtrPtr is an unsafe pointer wrapping C's `char**`.
	type CharPtrPtr unsafe.Pointer

	// CharPtr is an unsafe pointer wrapping C's `char*`.
	type CharPtr unsafe.Pointer

	// SizeTPtr is an unsafe pointer wrapping C's `size_t*`.
	type SizeTPtr unsafe.Pointer

rados: implement binding for rados_read_op_read #615

rados: implement binding for rados_read_op_read #615

Conversation

gman0 commented Dec 3, 2021

Checklist

gman0 commented Dec 3, 2021

gman0 commented Dec 3, 2021 • edited Loading

phlogistonjohn commented Dec 3, 2021

ansiwen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gman0 commented Dec 8, 2021

gman0 commented Jan 5, 2022 • edited by ansiwen Loading

ansiwen commented Jan 5, 2022

ansiwen commented Jan 5, 2022

gman0 commented Jan 7, 2022

phlogistonjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gman0 commented Jan 11, 2022

phlogistonjohn left a comment

Choose a reason for hiding this comment

ansiwen left a comment

Choose a reason for hiding this comment

gman0 commented Jan 13, 2022

phlogistonjohn commented Jan 13, 2022

gman0 commented Dec 3, 2021 •

edited

Loading

gman0 commented Jan 5, 2022 •

edited by ansiwen

Loading