Closed vslpsl closed 3 months ago
Let me add some initial comments:
ReadOp.Exec()
and WriteOp.Exec()
in a new file called _rw_opexec.go.//go:build ceph_preview
at the very beginning of every source file.make api-update
once you are done with the changes.Regarding the API stuff @anoopcs9 mentioned, it's documented here: https://github.com/ceph/go-ceph/blob/master/docs/development.md#api-status
cc @ansiwen especially WRT the new bufferlist structure
.... and thank you very much for looking into adding these apis!
cc @ansiwen especially WRT the new bufferlist structure
I'm on PTO until June 6, I can only have a look afterwards. But don't hesitate to merge it before. I can still propose a fixup, if I don't like something.
Thanks @ansiwen for the update. We'll see how it goes.
A couple of thoughts on the helper type:
Let me know if I have misunderstood, or misread, anything in the above - if there's value to the new type please share what the advantage is. Otherwise it may be better to return a native Go type. Note that you can still use the type (renamed to lowercase) as an implementation detail if you wish, in this case.
I should have looked closer at the test. This is part of the 'Op' infrasctructure that is a "batch" operations API. You don't have real bytes to return yet -- the buffer class is a bit of a "promise" rather than a simple "buffer". Let me think if I can come up with a better suggestion.
After refreshing my memory wrt the existing APIs, I think you can mimic what was done for the read api: https://github.com/ceph/go-ceph/commit/937f8d81640455cc79d1dc63a3a2ed76e19db068#diff-36918497c2d2778500619bb54b103c8fb67d60563f22f88b719afc675bf68a4dR61
Converge CephBufferList with the exec step to something like ReadOpExecStep
and it will become something similar to what we already have for Read. Does that make sense? Let me know if you have questions about this idea.
@mergifyio rebase
rebase
Rebased & applied changes proposed by @phlogistonjohn
Thanks for continuing to work on this. The code looks pretty good to me now. I think the only things remaining are "organizational".
First, I think the JSON generated from the api update make commands is out of date with the actual doc comments in the code. The simplest thing might be simply to delete the current changes and regenerate the JSON changes from scratch.
Related to the git commit history: We prefer a linear history and as such, do not want intermediate commits in the final history. If you are comfortable doing it, could you please squash all the commits on this PR into a single commit. While doing that, please add a Signed-off-by
trailer (see also) with the -s option.
If you don't feel comfortable doing this on your own feel free to ask us for steps or direct assistance...
Also, please rebase or configure mergify so that we can trigger rebases using mergify (see https://github.com/ceph/go-ceph/pull/989#issuecomment-2150067966 ).
I did rebase & squash & fixed api json.
@phlogistonjohn please review changes
Signed off by and commit message look ok to me on first glance. I'll enable the test run and try and re-review in the next couple of hours.
@anoopcs9 and/or @ansiwen - 2nd review please.
Added
rados_read_op_exec
&rados_write_op_exec
go-wrappers.Fixed: #307 Fixed: #305
Added
CephBufferList
structure to manage c++ allocated memory.