Re: librados compound operations

[Sage Weil <sage@xxxxxxxxxxxx>]

On Mon, 28 Mar 2011, Colin McCabe wrote:
> On Mon, Mar 28, 2011 at 3:46 PM, Sage Weil <sage@xxxxxxxxxxxx> wrote:
> > Below is a patch that wraps the internal Objecter compound ObjectOperation
> > so that we can send compound operations to the OSDs via librados.
> >
> > The internal ObjectOperationImpl ends up being unnecessary; I just cast it
> > to the internal type directly.
> >
> > One weird thing I noticed is with the constructor.  In Rados:: did
> >
> > +    static ObjectOperation *operation_create();
> >
> > and the user then deletes it when they're done (after using it for one or
> > more calls to operate() or aio_operate()).  Is that the the approach we
> > want?  Because right above that in the header is
> >
> >     int ioctx_create(const char *name, IoCtx &pioctx);
> >
> 
> I second TV's argument that returning a detailed error code is better
> than returning NULL or not-NULL.
> 
> I also don't like calling conventions that always use dynamic memory
> allocation. Even on a shiny new 16-core system, there is still a
> global mutex around new()/free(). I also like putting stuff on the
> stack and using RAII. (I haven't looked at this new code enough to see
> whether RAII makes sense here though.)

Hmm.  Well, pushing the work into the construct lets you just do 
something like

  ObjectOperation o;
  o.write_full(bl);
  o.setxattr("foo", bl2);
  int r = io_ctx.operate(oid, &o, &outbl);

which is cleaner IMO, but you don't get an error code.  For that, you need 
something like

  ObjectOperation o;
  if (rados.operation_init(o) < 0)
	...
  o.write_full(bl);
  o.setxattr("foo", bl2);
  int r = io_ctx.operate(oid, &o, &outbl);

I prefer the former, myself.

sage


diff --git a/src/include/rados/librados.hpp b/src/include/rados/librados.hpp
index 58e1fb6..a44a68a 100644
--- a/src/include/rados/librados.hpp
+++ b/src/include/rados/librados.hpp
@@ -98,6 +98,7 @@ namespace librados
   class ObjectOperation
   {
   public:
+    ObjectOperation();
     ~ObjectOperation();
 
     void write(uint64_t off, const bufferlist& bl);
@@ -114,7 +115,6 @@ namespace librados
 
   private:
     ObjectOperationImpl *impl;
-    ObjectOperation(ObjectOperationImpl *impl_) : impl(impl_) {}
     ObjectOperation(const ObjectOperation& rhs);
     ObjectOperation& operator=(const ObjectOperation& rhs);
     friend class IoCtx;
@@ -263,8 +263,6 @@ namespace librados
 
     int ioctx_create(const char *name, IoCtx &pioctx);
 
-    static ObjectOperation *operation_create();
-
     /* listing objects */
     int pool_list(std::list<std::string>& v);
     int get_pool_stats(std::list<std::string>& v,
diff --git a/src/librados.cc b/src/librados.cc
index 788e1cc..2fd093f 100644
--- a/src/librados.cc
+++ b/src/librados.cc
@@ -2729,9 +2729,10 @@ aio_create_completion(void *cb_arg, callback_t cb_complete, callback_t cb_safe)
   return new AioCompletion(c);
 }
 
-librados::ObjectOperation *librados::Rados::operation_create()
+
+librados::ObjectOperation::ObjectOperation()
 {
-  return new librados::ObjectOperation((ObjectOperationImpl *)new ::ObjectOperation);
+  impl = (ObjectOperationImpl *)new ::ObjectOperation;
 }
 
 librados::ObjectOperation::~ObjectOperation()
diff --git a/src/testradospp.cc b/src/testradospp.cc
index 971549a..1caa075 100644
--- a/src/testradospp.cc
+++ b/src/testradospp.cc
@@ -204,12 +204,11 @@ int main(int argc, const char **argv)
   }
 
   cout << "compound operation..." << std::endl;
-  ObjectOperation *o = rados.operation_create();
-  o->write(0, bl);
-  o->setxattr("foo", bl2);
-  r = io_ctx.operate(oid, o, &bl2);
+  ObjectOperation o;
+  o.write(0, bl);
+  o.setxattr("foo", bl2);
+  r = io_ctx.operate(oid, &o, &bl2);
   cout << "operate result=" << r << std::endl;
-  delete o;
 
   cout << "iterating over objects..." << std::endl;
   int num_objs = 0;