agent/txn_endpoint: configure max txn request length #7388
Conversation
agent/txn_endpoint.go
Outdated
| // maxTxnSize is the size limit (bytes) for for transaction request bodies. | ||
| // The default is set to the suggested raft size to keep the total transaction | ||
| // size reasonable to account for timely heartbeat signals. | ||
| maxTxnSize := int64(s.agent.config.TxnMaxDataSize) |
There was a problem hiding this comment.
If someone configured KVMaxValueSize, it would be useless after an upgrade. Could you change the code to handle the case where KVMaxValueSize is set too, maybe by falling back to it?
There was a problem hiding this comment.
great catch, i'll add a check to fall back if KV limit is greater than the txn limit
agent/txn_endpoint.go
Outdated
| // transactions are automatically set to attempt a chunking apply. | ||
| // Performance may degrade and warning messages may appear. | ||
| if maxTxnSize == 0 { | ||
| maxTxnSize = raft.SuggestedMaxDataSize |
There was a problem hiding this comment.
nit: The default of KVMaxValueSize is set in agent/config/default.go. I would find it easier if this default would be in the same spot, to make it easier to see that they are the same (I just looked it up). Or adding a comment would also help.
There was a problem hiding this comment.
that's fair! i see now that all the defaults are in the same location -- moved 👍
|
|
||
| // Check Content-Length first before decoding to return early |
There was a problem hiding this comment.
I know this code existed before, but I don't like the duplication and I am wondering what your opinion is on removing the early return in favor of readability and maintainability.
There was a problem hiding this comment.
If availability of the agents are important, I think it's worth while to reduce the overhead of unnecessary processing time to decode a large request. Though, on the point of readability, what do you think about the change I just pushed up? It simplifies checking of content length from relying on the Content-Length header to using the actual request length.
agent/config/runtime_test.go
Outdated
| @@ -6217,6 +6217,7 @@ func TestSanitize(t *testing.T) { | |||
| "StatsiteAddr": "" | |||
| }, | |||
| "TranslateWANAddrs": false, | |||
| "TxnMaxDataSize": 0, | |||
There was a problem hiding this comment.
There should be two more places in this file where you are setting a random value in json and hcl configs and check the result afterwards.
There was a problem hiding this comment.
Appreciate it! Everything should be added now after following the config field guide
findkim
changed the title
configure max transaction size separately from kv limit
configure max transaction size separately from kv limit
configure max transaction size separately from kv limit
This PR introduces the option to configure the maximum request length for transaction requests to the
/txnendpoint that is distinct from the KV value limit. These changes do not affect the current default behavior of the/txnendpoint and aims to improve clarity on the limits imposed on the transaction by narrowing the usage of the KV value limit to only KV operations.limits.txn_max_req_len