librados api cleanup

librados api cleanup

[Sage Weil]

There are a few problems with the current librados api (at least the C 
bindings).  The main one is that there is an implicit assumption that you 
can only interact with a single cluster from the same process.  
rados_initalize() takes argc/argv, allowing you to pass in configuration 
options and/or a config file location, and it is assume that is the 
cluster that is referenced when you open up a pool.

Beyond that, there are some cosmetic issues with function naming, argument 
ordering, and so forth.  If we're going to fix the main problem and break 
compatibility, we should take the opportunity to clean up the rest.  And 
make the change sooner rather than later to minimize the pain.

See http://tracker.newdream.net/issues/801 and the librados_api branch in 
ceph.git.

In order to make it really work the way it looks, though, we need to 
identiy and eliminate as many remaining instances of common static 
variables.  Currently, for example, the configuration state is static 
(in this case, shared between clusters).  For things like 
debugging/logging, that's fine, but for other options it can be 
problematic.

Are there other problems that people have run into?  How important are 
these issues?  At the very lease we can stick this stuff on the long term 
roadmap.

sage

Re: librados api cleanup

[Colin McCabe]

On Mon, Feb 14, 2011 at 3:00 PM, Sage Weil <sage@newdream.net> wrote:
> There are a few problems with the current librados api (at least the C
> bindings).  The main one is that there is an implicit assumption that you
> can only interact with a single cluster from the same process.
> rados_initalize() takes argc/argv, allowing you to pass in configuration
> options and/or a config file location, and it is assume that is the
> cluster that is referenced when you open up a pool.

One idea would be to have a C API like this:

rados_cluster_t ctx;
rados_initialize(&ctx);
rados_read_configuration(ctx, "my_ceph_config.conf");
rados_set_conf("log dir", "/my/log/dir");

[leaving out error handling for brevity]

So essentially, you have this rados_cluster_t which encapsulates the
cluster you're talking to and the configuration options you're using.
It should be fairly easy to create a function like rados_set_conf,
which sets arbitrary configuration options to new values in case
library users want to force particular settings.

The nice thing about doing it like this, rather than adding arguments
to rados_initialize, is that it's open-ended. We won't need to keep
creating more and more versions of rados_initialize as people discover
new options they want to set.

> In order to make it really work the way it looks, though, we need to
> identiy and eliminate as many remaining instances of common static
> variables.  Currently, for example, the configuration state is static
> (in this case, shared between clusters).  For things like
> debugging/logging, that's fine, but for other options it can be
> problematic.

I think we should bite the bullet and put the configuration into a
ceph_context_t.

Then to cosd.cc, cmon.cc, etc, we add
ceph_context_t *g_ctx;

Then the daemon code can be converted just by doing:
s/g_conf/g_ctx->conf/g

The code that's shared between daemons and libraries would need to add
ceph_context_t as a parameter to certain functions. The linker will
find any "mistakes" since references to g_ctx will not be resolved in
non-daemon code.
So it may be easier than it seems.

cheers,
Colin
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: librados api cleanup

[Sage Weil]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1380 bytes --]

On Mon, 14 Feb 2011, Colin McCabe wrote:

> On Mon, Feb 14, 2011 at 3:00 PM, Sage Weil <sage@newdream.net> wrote:
> > There are a few problems with the current librados api (at least the C
> > bindings).  The main one is that there is an implicit assumption that you
> > can only interact with a single cluster from the same process.
> > rados_initalize() takes argc/argv, allowing you to pass in configuration
> > options and/or a config file location, and it is assume that is the
> > cluster that is referenced when you open up a pool.
> 
> One idea would be to have a C API like this:
> 
> rados_cluster_t ctx;
> rados_initialize(&ctx);
> rados_read_configuration(ctx, "my_ceph_config.conf");
> rados_set_conf("log dir", "/my/log/dir");

How about:

---
/* initialization */
int rados_open(rados_t *cluster);
void rados_close(rados_t cluster);

/* config */
int rados_conf_parse_argv(rados_t cluster, int argc, char **argv);
int rados_conf_readfile(rados_t cluster, const char *path);
int rados_conf_set(rados_t cluster, const char *option, const char *value);
const char *rados_conf_get(rados_t cluster, const char *option);
---

I think the (short term) trick will be supporting this immediately before 
the config refactoring is in place.  Should be possible with a hidden 
argv vector and the existing internal framework, I think. 

sage

Re: librados api cleanup

[Colin McCabe]

On Fri, Feb 18, 2011 at 1:36 PM, Sage Weil <sage@newdream.net> wrote:
> [config example]

Looks good.

> const char *rados_conf_get(rados_t cluster, const char *option);

I would prefer something like
int rados_conf_get(rados_t cluster, const char *option, char **buf, int len);

We have a lot of configuration options that are ints and floats. You
can't really return a const char* pointer to an int-- you need to make
a temporary string somewhere, and that won't be const...

We should also provide some feature like "pass -1 as length, and we'll
malloc it for you."

> I think the (short term) trick will be supporting this immediately before
> the config refactoring is in place.  Should be possible with a hidden
> argv vector and the existing internal framework, I think.

Yeah, it should be do-able.

Colin
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: librados api cleanup

[Tommi Virtanen]

On Fri, Feb 18, 2011 at 02:49:37PM -0800, Colin McCabe wrote:
> > const char *rados_conf_get(rados_t cluster, const char *option);
> 
> I would prefer something like
> int rados_conf_get(rados_t cluster, const char *option, char **buf, int len);
> 
> We have a lot of configuration options that are ints and floats. You
> can't really return a const char* pointer to an int-- you need to make
> a temporary string somewhere, and that won't be const...

The usual idiom is to have a low-level "raw" string getter, then build
rados_conf_get_uint32, rados_conf_get_bool (e.g. understands
"yes"/"true" and "no"/"false") etc on top of that.

-- 
:(){ :|:&};: