make read txn timeout configurable and set default to 1min#304
make read txn timeout configurable and set default to 1min#304
Conversation
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Thank for getting this configuration in!
| const trackedTxns: WeakRef<any>[] = []; | ||
| setInterval(() => { | ||
| const configValue = envMngr.get(CONFIG_PARAMS.STORAGE_MAXREADTRANSACTIONOPENTIME) ?? 60000; | ||
| let READ_TXN_TIMEOUT_TICKS = Math.round(Math.min(Math.max(configValue, 15000), 300000) / 15000); // clamp between 15s and 5min |
There was a problem hiding this comment.
I don't think we actually need to clamp this. I know I had mentioned concerns about excessively long timeouts here, but that was more of a consideration for those setting the value, but I trust that we can set the value properly. And I think that a default of one minute is too short. I believe the previous setting was 15 minutes, right? Maybe the default should be 5 minutes?
cb1kenobi
left a comment
cb1kenobi
left a comment
There was a problem hiding this comment.
Code looks fine, but the title of the PR is confusing. It says "1min", though I don't see any 1 minute timeouts.
| STORAGE_ENCRYPTION: 'storage_encryption', | ||
| STORAGE_MAXTRANSACTIONQUEUETIME: 'storage_maxTransactionQueueTime', | ||
| STORAGE_MAXTRANSACTIONOPENTIME: 'storage_maxTransactionOpenTime', | ||
| STORAGE_MAXREADTRANSACTIONOPENTIME: 'storage_maxReadTransactionOpenTime', |
There was a problem hiding this comment.
Can we add some more underscores? This contant is not very readable.
| STORAGE_MAXREADTRANSACTIONOPENTIME: 'storage_maxReadTransactionOpenTime', | |
| STORAGE_MAX_READ_TRANSACTION_OPEN_TIME: 'storage_maxReadTransactionOpenTime', |
5 participants
Is 1min as default ideal, or should we set another timeout?