-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add transaction size option #1725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
alecgrieser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good overall, with just a few things. Were you able to verify this by trying to write a transaction that was larger than same custom option but still less than 10 MB and getting a failure?
|
I've addressed all comments and added a python test that verified the size limit is effective. The test catches a bug of not setting per transaction limit, which was fixed. |
alecgrieser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. I had two more comments on the docs that I left in the form of change suggestions.
Co-Authored-By: Alec Grieser <[email protected]>
Co-Authored-By: Alec Grieser <[email protected]>
This allows client to set transaction size as a database option, or as a transaction option.
This fixes #1681.