go-ceph icon indicating copy to clipboard operation
go-ceph copied to clipboard

MonCommand functions incorrectly accept only a single byte slice

Open phlogistonjohn opened this issue 5 years ago • 2 comments

As discussed in PR #99 the Ceph API accepts an array of c-strings but currently the wrapper functions only accepts a single byte-slice and hard codes the c call only accept on string. See: https://github.com/ceph/go-ceph/blob/281fccac99ebb6c8b24c169c39c59d212a3df61c/rados/conn.go#L284

We should not artificially limit the api calls to one slice, but we also may want to ease the transition from the current function spec as we might have existing code calling this API. Something to consider.

phlogistonjohn avatar Dec 13 '19 16:12 phlogistonjohn

We're probably going to start making use of the function ousrelves in cephfs/admin for any functionality not currently handled directly by a mgr command (there appear to be a few).

Since MonCommand is "old" it takes []byte instead of [][]byte like most of the other *Command functions. The latter was done to better match the ceph apis, but honestly it wasn't entirely necessary given how ceph makes use of the arguments ( sigh ).

Regardless of the reason, I feel we should aim to be consistent. But I also care about not breaking existing users. Here's a few approaches:

  • Just change MonCommand now. Anyone directly consuming master will break and there's a breaking api change at v0.6.
  • Announce deprecation of current MonCommand at v0.6 time and then change it for v0.7. We at least give some advance warning. It means that cephfs/admin interface types will probably need to follow along possibly breaking any code written for v0.6, but that probably won't be too many codebases if any.
  • Add MonCommand2 (or some other tweaked name) that has the desired signature, leave MonCommand alone but deprecate it in v0.6 with a promise to drop it in some future version. In that version MonCommand and MonCommand2 are essentially the same function. We'd probably drop MonCommand2 in favor of the MonCommand at v1.0 (but that is very negotiable).
  • Stop worrying and learn to love the inconsistency. Close this issue and don't sweat the api differences. :-)

Please feel free to suggest anything else. I'd like to pick one of the above in the next few (3 probably) weeks or so. Then we can go ahead with implementation and documentation.

^ @nixpanic @agarwal-mudit @ansiwen feedback greatly desired, thanks!

phlogistonjohn avatar Aug 20 '20 19:08 phlogistonjohn

I personally prefer bullet 4 - MonCommand2 - but I am willing to admit I'm often overly fond of backwards compatibility. But in this case I do think the maintenance effort is probably worth the cost. I happen to know there are some outside projects currently using this call.

phlogistonjohn avatar Aug 20 '20 19:08 phlogistonjohn