In order to differentiate commits from fences better, create
a new set of request callbacks kvs.fence and kvs.relayfence.
In libkvs, have commits call kvs.commit and have fences call
kvs.fence appropriately.

This comment has been minimized.

edited

Diff coverage doesn't look to good (63.82%) but looking through the coverage, its predominantly "impossible" error path coverage being missed. Not sure I'll be able to eek out better coverage with unit tests.

This comment has been minimized.

I'd probably drop the commit that diverts fence to commit if fence nprocs=1, since that left turn isn't likely justified by any real world benefit.

In the commit request callback, it looks like a uuid is still being created, but on the server side. Is that necessary? If it is, it might actually be better to do it on the client side to distribute the workload rather than concentrating it on the leader rank.

Slightly unrelated to this PR, but the duplication of the rpc call in the client (my fault) looks to be just to avoid creating an empty ops array? I'm not sure that works out to be helpful, since jansson probably just creates it internally from [] before serializing anyway. It's a bit ugly and might be useful to fix while you're in there.

This comment has been minimized.

In the commit request callback, it looks like a uuid is still being created, but on the server side. Is that
necessary

Yeah. It's b/c there is shared code for commits & fences returning success/error codes to the original callers. I need a name to attach to an RPC message for later lookup. This is all in the finalize_transactions_bynames() path.

If it is, it might actually be better to do it on the client side to distribute the workload rather
than concentrating it on the leader rank.

That sounds like a good idea.

Slightly unrelated to this PR, but the duplication of the rpc call in the client (my fault) looks to be just to avoid creating an empty ops array?

Yup.

I'm not sure that works out to be helpful, since jansson probably just creates it internally from [] before serializing anyway. It's a bit ugly and might be useful to fix while you're in there.

Are you suggesting we create an empty ops array if the user passes in txn == NULL? I suppose that would clean it up a bit overall.

In kvstxn_mgr_process_transaction_request(), do not check/set
processing flag and do not check if transaction count has
reached nprocs. Make that something the caller is required
to do.
This separates treq logic out of the kvstxn API far more.
Adjust core KVS file appropriately. Most notably, in fences
calls to treq_count_reached() must be checked, but in commits
they do not.

Refactor kvstxn_mgr_process_transaction_request() into
kvstxn_mgr_add_transaction(). The new function takes direct
parameters needed for a ready transaction (i.e. name, ops, flags).
With this refactoring, treq_t transactions are now completely
decoupled from the kvstxn_t API. So remove now unnecessary header
includes and object linking.

In relaycommit_request_cb(), do not create a treq data structure.
As commits are individually unique, it is not necessary. Instead,
pass ops, name, and flags to kvstxn_mgr_add_transaction()
directly.
In commit_request_cb(), ops do not need to be stored as commits
are unique.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.