API: overhaul KVS interfaces

[garlick]

The primary KVS interfaces should operate on valid JSON strings, not json-c specific json_object types, like the redesigned message functions.

[garlick]

This is not a fully formed idea but it seems useful for kvs_get() and kvs_watch() functions to adopt an interface similar to flux_rpc(). This would enable:

• returning a const char *json_str where storage of the returned value is held in the response message, which in turn is held in the flux_rpc_t like object
• these functions to be used asynchronously with check/get/then interfaces, straightening out the confusing and clunky kvs_watch() and kvs_watch_once() calls.

Could we just use a flux_rpc_t directly? E.g.

flux_rpc_t *kvs_get (flux_t h, const char *key);
// can use flux_rpc_check() directly
int kvs_rpc_get (flux_rpc_t *rpc, const char **json_str);
// add get variants for different types?
// can use flux_rpc_then() directly
// can use flux_rpc_destroy() directly

[garlick]

Watch could just be a variant of kvs_get() with the rest identical (could even be a flag to kvs_get().

The RPC calls would need to be modified to support multiple responses (and multiple gets) as described in #271

[garlick]

In order for kvs_put() and kvs_commit() to be used asynchronously, they could be similarly wrapped. In that case the kvs_rpc_get() call would just return the error status (or commit may be modified to return a snapshot reference).

flux_rpc_t *kvs_put (flux_t h, const char *key, const char *json_str);
flux_rpc_t *kvs_commit (flux_t h);

[grondo]

Hm, the get in flux_rpc_get is unfortunate name choice when it comes to kvs operations, the naming here is turning out a bit confusing imo. It will be a bit difficult to read code with kvs_get and kvs_rpc_gets sprinkled around.

It does seem to me like you are on to something here though. It would be really nice to be able to extend the generic rpc pattern to other services that for whatever reason need to wrap the rpc interface. That would make adding C functions wrapping the protocols much easier, as well as ensuring a consistent interface.

Oh and your proposal to have simple rpcs kvs_put kvs_commit return flux_rpc_t seems exactly right to me. In general all simple rpc wrappers should follow this pattern I would think.

[garlick]

Yeah sorry, the naming above is definitely poor. For the moment I'm kind of stumped on finding good names though - it seems like if we are wrapping the flux_rpc() and flux_rpc_get() calls and allowing the other flux_rpc_t calls to be used directly, then we need to make clear in the naming both 1) the operation (kvs_get) and 2) the RPC call being wrapped (flux_rpc_get). I suppose a consolation is that this is probably about the worst case we'll have if we go this way for other flux-core RPC's. Example, flux_info() becomes flux_info() and flux_info_get() - no problem!

[garlick]

Is this a slight improvement or still "meh..."?

flux_rpc_t *kvs_request (flux_t h, const char *key);
int kvs_get_json (flux_rpc_t *rpc, const char **json_str);
int kvs_get_int (flux_rpc_t *rpc, int *val);
etc

A synchronous get would look like:

flux_rpc_t *rpc;
const char *json_str;
if (!(rpc = kvs_request (h, "foo.bar.baz"))
    err_exit ("kvs_request");
if (kvs_get_json (rpc, &json_str) < 0)
    err_exit ("kvs_get_json");
flux_rpc_destroy (rpc); /* invalidates json_str */

I do like the fact that the rpc "owns" the storage associated with the response. In the current KVS api, some returned values have to be freed, others not. That aspect at least would be an improvement. And of course this makes async programming possible should you want to issue the get and do other things before the result arrives.

[grondo]

I agree that the rpc object ownership of storage is a great improvement. I think this is a really nice development. What you have above is a huge usability improvement, if you want my opinion.

If you call kvs_get_json on an rpc object not created by kvs_request, will you get EINVAL?
It might be nice if rpc objects had a type associated so you could do

 error ("kvs_get_json() called on rpc object of type %s\n", flux_rpc_type (rpc))

I dunno, maybe that isn't particularly useful, but I like this idea of lightweight extensible type system for rpc objects....

[garlick]

OK, thanks for the encouragement. I'll think a bit more along these lines then.

Good idea giving flux_rpc_t objects a type. I was also thinking about adding a way to store opaque data in there with a destructor so that complex value types like kvsdir_t could be allocated internally in the get function and returned to the caller, but still destroyed by flux_rpc_destroy().

[garlick]

Revisiting the API rework, building on futures and jansson style varargs interfaces. Consider this a straw man proposal for put and get interfaces, feedback welcome.

put for single key (commit implied):

flux_future_t *flux_kvs_put (flux_t *h, const char *key, const char *json_str);
flux_future_t *flux_kvs_putf (flux_t *h, const char *key, const char *fmt, ...);

// finish with flux_future_get (f, NULL) or if commit sequence number needed:
int flux_kvs_get_seq (flux_future_t *f, int *seq);

put for one or more keys (atomic transaction, with fence capability):

flux_future_t *flux_kvs_txopen (flux_t *h);

int flux_kvs_txput (flux_future_t *f, const char *key, const char *json_str);
int flux_kvs_txputf (flux_future_t *f, const char *key, const char *fmt, ...);

int flux_kvs_txcommit (flux_future_t *f, int flags);
int flux_kvs_txfence (flux_future_t *f, const char *name, int nprocs);

// finish with flux_future_get (f, NULL) or if commit sequence number needed
// use flux_kvs_get_seq() above

get:

flux_future_t *flux_kvs_lookup (flux_t *h, const char *key, int flags);

int flux_kvs_get (flux_future_t *f, const char *key, const char **json_str);
int flux_kvs_getf (flux_future_t *f, const char *key, const char *fmt, ...);

For the get I was thinking of flags that would allow a recursive get in one RPC and then accessors to retrieve individual key/values from the result. Would need interfaces to walk the returned key space also, but I didn't get that far.

[grondo]

For the atomic transactions, I wonder if this is a case where a new type would work better than overloading the future_t. It is a bit strange to be calling put, commit, and fence on a future, which is a container for a deferred result. If you had the txopen return a flux_kvs_txn_t, then you could put, commit, and fence on this object, and finally close it. Under the covers it would presumably be built on flux_future_t?

[garlick]

That might be a good idea. It would mean flux_kvs_txcommit() and flux_kvs_txfence() (or equivalent) would accept a flux_kvs_txn_tand return a future, so that flux_future_wait_for() and flux_future_then() could be used for synchronization. Yeah, I think I like that (will edit my propsoal when I get in).

[morrone]

I like @grondo's use of "txn" rather than "tx", and I think that would be good to use in the function names as well. I would like to see an underscore between "txn" in the function name and the following term (e.g. flux_kvs_txn_commit()).

I am not sure that I understand the semantics of doing a fence on an atomic transaction though. Could you elaborate on what you have in mind there?

Also, I assume that other forms of put, such as "_put_int", "_put_double" will also exist? In that case, shouldn't "flux_kvs_put" be named "flux_kvs_put_json"?

[garlick]

Regarding txn function naming, I agree. Instead of editing my comment, I will open a new issue so we can iterate in the description and dump the old discussion here that is not really relevant anymore.

I am not sure that I understand the semantics of doing a fence on an atomic transaction though. Could you elaborate on what you have in mind there?

The fence essentially combines nprocs commits into one transaction. So while the flux_kvs_txn_t in that case is not the whole transaction, that batch of changes is still an all or nothing proposition, so stll appropriate?

Also, I assume that other forms of put, such as "_put_int", "_put_double" will also exist?

No I think flux_kvs_txn_putf() would cover that. E.g. to set key foo to the integer value 42:

flux_kvs_txn_putf (txn, "foo", "i", 42);
// or
flux_kvs_putf (h, "foo", "i", 42);

[SteVwonder]

Is there anything else left from this discussion to complete? Or can this issue be closed?

[garlick]

I think we can close it.