rework KVS interface

[garlick]

As started in #76, proposed new KVS interfaces

Transactions:

typedef struct flux_kvs_txn flux_kvs_txn_t;

flux_kvs_txn_t *flux_kvs_txn_create (void);
void flux_kvs_txn_destroy (flux_kvs_txn_t *txn);

int flux_kvs_txn_put (flux_kvs_txn_t *txn, int flags,
                      const char *key, const char *json_str);

int flux_kvs_txn_putf (flux_kvs_txn_t *txn, int flags,
                       const char *key, const char *fmt, ...);

int flux_kvs_txn_mkdir (flux_kvs_txn_t *txn, int flags,
                        const char *key);

int flux_kvs_txn_unlink (flux_kvs_txn_t *txn, int flags,
                         const char *key);

int flux_kvs_txn_symlink (flux_kvs_txn_t *txn, int flags,
                          const char *key, const char *target);

Commit/fence:

flux_future_t *flux_kvs_commit (flux_t *h, flux_kvs_txn_t *txn, int flags);

flux_future_t *flux_kvs_allcommit (flux_t *h, flux_kvs_txn_t *txn, int flags,
                                   const char *name, int nprocs);

int flux_kvs_commit_get_seq (flux_future_t *f, int *seq);

int flux_kvs_commit_get_root (flux_future_t *f, const char **json_str);

Convenience (put):

/* Convenience functions, equivalent to e.g.
 *   flux_kvs_txn_t *txn = flux_kvs_txn_create()
 *   flux_kvs_txn_put(txn, flags, key, val)
 *   flux_future_t *f = flux_kvs_commit (h, txn, flags);
 *   flux_kvs_txn_destroy(txn);
 */

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

flux_future_t *flux_kvs_putf (flux_t *h, int flags,
                              const char *key, const char *fmt, ...);

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

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

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

Get:

// FILL IN

Watch:

// FILL IN

[morrone]

I think I'd like to see a term other than "putf", since the format doesn't match what we'd expect to see in all of the other "f" functions in the standard C library.

[garlick]

I thought "verb+f" was probably the expected form. What would you suggest?

[chu11]

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

Would we want to return flux_future_t for these convenience functions? I would imagine the common case for users would be:

f = flux_kvs_put (...)
flux_future_get (f, NULL)
flux_future_destroy (f);

We could probably just wrap the get & destroy in flux_kvs_put too.

[garlick]

As suggested in #353, flux_kvs_lookup() should accept a flag that tells it to fetch a directory recursively. The future (as container) makes it easy to store the result for retrieval with accessors, but I'm struggling with what those accessors look like.

Is it useful to treat the result as a flat list of fully qualified keys that can be iterated over and individually accessed? For example:

const char *flux_kvs_lookup_first (flux_future_t *f);
const char *flux_kvs_lookup_next (flux_future_t *f);

and then add a key parameter to get, e.g.:

int flux_kvs_lookup_get (flux_future_t *f, const char *key, const char **json_str);
int flux_kvs_lookup_getf (flux_future_t *f, const char *key, const char *fmt, ...);

Or do we need something more like libutil/dirwalk?

[grondo]

@chu11, hopefully users are actually using the reactive model (especially in modules), so I would assume the common case is more like:

 f = flux_kvs_put (...);
 flux_future_then (f, 0, handle_error, NULL);
 /* Back to event loop */

[garlick]

Would we want to return flux_future_t for these convenience functions? I would imagine the common case for users would be:
f = flux_kvs_put (...)
flux_future_get (f, NULL)
flux_future_destroy (f);
We could probably just wrap the get & destroy in flux_kvs_put too.

That's how the current API works, and the problem is that it forces the user to make a synchronous RPC, which is not good practice in reactor context. It's better to expose the future and let the user decide IMHO.

[chu11]

That's how the current API works, and the problem is that it forces the user to make a synchronous RPC, which is not good practice in reactor context. It's better to expose the future and let the user decide IMHO.

Yeah, I guess I was thinking common case like the flux-kvs command, where synchronous is all a user would desire. The more advanced asynchronous work would be in modules and the user would use the full txn api. It's a minor pro-con tradeoff either way.

[grondo]

As suggested in #353, flux_kvs_lookup() should accept a flag that tells it to fetch a directory recursively. The future (as container) makes it easy to store the result for retrieval with accessors, but I'm struggling with what those accessors look like.

Would it work to get some kind of kvs-specific object from the future_t, instead of operating directly on the future? That way, you can implement something recursive without returning a future_t from a future_t.

E.g. flux_kvs_lookup_get() could return something like a flux_kvs_dir_t * (or maybe flux_kvs_namespace_t), upon which you could iterate keys, or open a "subdir" as a new flux_kvs_dir_t *. Maybe eventually the API could allow puts into the namespace and have it separately committed or linked into a new namespace with the KVS namespaces support.

I'm not sure if this is a good or dumb idea actually.

[morrone]

I thought "verb+f" was probably the expected form. What would you suggest?

I would expect that function to operate like printf, or scanf, or any of the others. But that's not how it works, right? It is going to be jansson-style formatting. So...anything but named like the C formatting functions would be fine with me.

[grondo]

Maybe instead of putf, getf, use the jansson naming pack, unpack? Not sure if flux_kvs_pack() telegraphs what it is doing as well as putf() though...

[garlick]

Yeah, I guess I was thinking common case like the flux-kvs command, where synchronous is all a user would desire. The more advanced asynchronous work would be in modules and the user would use the full txn api. It's a minor pro-con tradeoff either way.

That's a good point (that the txn interface already exposes the future). I don't know I guess I could go either way on that one. It's certainly true that replacing existing flux_kvs_put() calls with the future interface will inflate the code somewhat. On the other hand, exposing the futures sets the stage for converting synchronous RPCs to continuations in the many places where they are made inappropriately.

I lean towards keeping it this way, but I'm not strongly committed if other people felt differently.

[garlick]

Maybe instead of putf, getf, use the jansson naming pack, unpack? Not sure if flux_kvs_pack() telegraphs what it is doing as well as putf() though...

Yeah, I'm not sure what would be better. If we come up with something better we should open an issue to go back and fix the various payload encode/decode functions in message.h, request.h, response.h, event.h, rpc.h, mrpc.h, etc. to be consistent.

[grondo]

It's certainly true that replacing existing flux_kvs_put() calls with the future interface will inflate the code somewhat.

I'm not sure it would inflate code too much -- something like flux-kvs that is dead set on using all synchronous calls could just include a few short static wrapper functions and the rest of the code would remain the same.

My opinion is it is probably good to vector users toward reactive programming model anyway, and therefore the "convenience" functions should be at least usable by default in that mode.

[morrone]

Yeah, I'm not sure what would be better.

How about: flux_kvs_txn_putp()

p for pack.

But I'm not tied to that. Really, anything that avoids misleading me into thinking I can do "%d" would be fine.

[garlick]

How about: flux_kvs_txn_putp()

p for pack.

Not bad... although it telegraphs something else to LISP people (beerP?)

[garlick]

It may be least confusing to just append _pack and _unpack (for "..." args); _pack_ex and _unpack_ex (for va_list args). Then for those familiar with the jansson API, it's pretty obvious what's going on.

We would want to go back through the flux API and make all the variadic message parsing functions look the same, e.g.

• flux_msg_set_jsonf() becomes flux_msg_set_json_pack()
• flux_msg_vset_jsonf() becomes flux_msg_set_json_pack_ex()
• flux_msg_get_jsonf() becomes flux_msg_get_json_unpack()
• flux_msg_vget_jsonf() becomes flux_msg_get_json_unpack_ex()
• flux_rpcf() becomes flux_rpc_pack()
• flux_rpc_getf() becomes flux_rpc_get_unpack()

It's more verbose, but if it improves clarity, maybe an improvement...

[garlick]

Could probably drop the "set_json/get_json" portion in the flux_msg functions above actually, e.g. flux_msg_pack(), flux_msg_unpack().

[garlick]

Er, I should have said "vpack" not "pack_ex". The jansson function that accepts a va_list argument is json_vpack_ex() but we wouldn't want to allow the extended json_error_t arg as that would require users to include jansson.h, whereas now they only need it if they are doing something with json_t objects.

Sorry for the monologue.

[garlick]

I think the basic changes proposed in this bug have been merged now. Not "convenience functions" yet but all the substantive discussion about lookup, txns, and varargs naming has been addressed, so closing.