kvs: could use some refactoring

[chu11]

I think the main kvs.c in the kvs module could use some refactoring, it's just getting big (north of 3.3K lines).

It's not super obvious to me what could be improved at the moment, as we've parsed out most transactions, lookups, etc. into individual mini-APIs already.

One thought that comes to mind is perhaps a "kvs_config" that can deal with config stuff as that expands and grows (#6125, #6112) as a nice chunk of that is concentrated in the main kvs.c rather than the sub-APIs.

It's worth noting that care was made in the past to avoid "circular struct dependencies" in the KVS code, as it did lead to problems. In some cases just having "back pointers" to other structs could improve things, but it would lead to those circular dependencies. Thus why refactoring would be good.

[garlick]

I'd like to see more abstraction. Having classes with public structs containing references to other public structs creates a large mental burden for someone trying to follow things. I'm thinking of kvstxn.h and kvsroot.h mainly I guess.

It's hard to come up useful abstractions though.

[chu11]

Yeah, perhaps might be a good idea to look at all the kvs code and see if it could be abstracted in some (possibly poor choice of words) "holistic" way. Much of the current code was refactored sort of based on the original design, then namespace (and thus roots) were added on top, etc. etc. So there may be a smarter way to abstract it nowadays.

[chu11]

I had what I thought was a mini-epiphiny today that actually went nowhere, but wanted to document so I don't forget later.

Now that the KVS fence support is removed, I thought it would be possible to refactor / re-architect the reporting of errors to transactions. Since all transactions are now defined to come from a single commit request vs multiple requests in a fence, perhaps we do not have to report errors via "events" anymore.

However, due to the fact transactions can be merged, the errors via events is still needed. i.e. KVS stuffs can still happen to multiple transactions "at the same time" even though fencing isn't supported.