OSD class method execution

[vslpsl]

Added rados_read_op_exec & rados_write_op_exec go-wrappers.

Fixed: #307
Fixed: #305

Added CephBufferList structure to manage c++ allocated memory.

[anoopcs9]

Let me add some initial comments:

• New APIs are expected in separate source file with corresponding dedicated test file.
  • You can have both ReadOp.Exec() and WriteOp.Exec() in a new file called rw_op_exec.go.
• All new APIs are introduced as preview APIs indicated with the help of build tags.
  • Add //go:build ceph_preview at the very beginning of every source file.
• Run make api-update once you are done with the changes.

[anoopcs9]

I'm not sure but can we consider combining exec_step_op.go and ceph_buffer_list.go?

[phlogistonjohn]

Regarding the API stuff @anoopcs9 mentioned, it's documented here: https://github.com/ceph/go-ceph/blob/master/docs/development.md#api-status

[phlogistonjohn]

cc @ansiwen especially WRT the new bufferlist structure

[phlogistonjohn]

.... and thank you very much for looking into adding these apis!

[ansiwen]

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.

[phlogistonjohn]

Thanks @ansiwen for the update. We'll see how it goes.

A couple of thoughts on the helper type:

• Uppercased types and functions that are part of files in the 'rados' package would become part of the public api. So that would include: NewCephBufferList, CephBufferList.Bytes
• A new public API should be made preview first
• The public ReadOp.Exec returns a CephBufferList. Making CephBufferList a mandatory part of the public API
• Items in the 'internal' APIs are only for APIs that are genuinely "internal" to go-ceph and can not be exposed to the public API
• I'm not sure if the CephBufferList is strictly needed. It has one method Bytes, that returns a bytes slice
• What would be the downsides of calling .Bytes() within Exec and returning []byte from Exec?

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.

[phlogistonjohn]

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.

[phlogistonjohn]

After refreshing my memory wrt the existing APIs, I think you can mimic what was done for the read api:
937f8d8#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.

[phlogistonjohn]

@Mergifyio rebase

[mergify]

rebase

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
@odarix needs to authorize modification on its head branch.

[vslpsl]

Rebased & applied changes proposed by @phlogistonjohn

[phlogistonjohn]

Looking quite good now. When I start nitpicking the doc comments you know you're generally on the right track ;-)
I have one small but substantial comment on the Bytes method as I want to ensure it does something sensible if it is (incorrectly) called before Operate.

[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...

[phlogistonjohn]

Also, please rebase or configure mergify so that we can trigger rebases using mergify (see #989 (comment) ).

Initial review comments are no longer valid with latest version of the change.

[vslpsl]

I did rebase & squash & fixed api json.

[vslpsl]

@phlogistonjohn please review changes

[phlogistonjohn]

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.

[phlogistonjohn]

LGTM. Thanks so much for working with us on this.

[phlogistonjohn]

@anoopcs9 and/or @ansiwen - 2nd review please.

[anoopcs9]

lgtm, thanks.