-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
jobs: bump default progress log time to 30s #25791
Conversation
I'm assuming that this effectively fixes the Review status: 0 of 1 files reviewed at latest revision, all discussions resolved. pkg/sql/jobs/progress.go, line 26 at r1 (raw file):
nit: Comments from Reviewable |
@nvanbenschoten how long does it take for that workload command to fail? it's not going quickly for me in my tests so far when i'm verifying that master fails. |
It took around 20 minutes to fail before, but I'd let the command finish successfully (~3 hours) before concluding that this fixes the issue completely. |
This didn't appear to fix the problem. I'm going to work on a test that can reproduce this faster. |
You could try dropping the range size so that the row doesn't need to grow as large to trigger the error. |
The previous code allowed updates to be performed every 1s, which could cause the MVCC row to be very large causing problems with splits. We can update much more slowly by default. In the case of a small backup job, the 5% fraction threshold will allow a speedier update rate. Remove a note that's not useful anymore since the referred function can now only be used in the described safe way. See #25770. Although this change didn't fix that bug, we still think it's a good idea. Release note: None
I've removed the Fixes line so that this PR no longer closes the original bug. But I still think this is a good idea to merge anyway. |
bors r+ |
25014: storage: queue requests to push txn / resolve intents on single keys r=spencerkimball a=spencerkimball Previously, high contention on a single key would cause every thread to push the same conflicting transaction then resolve the same intent in parallel. This is inefficient as only one pusher needs to succeed, and only one resolver needs to resolve the intent, and then only one writer should proceed while the other readers/writers should in turn wait on the previous writer by pushing its transaction. This effectively serializes the conflicting reader/writers. One complication is that all pushers which may have a valid, writing transaction (i.e., `Transaction.Key != nil`), must push either the conflicting transaction or another transaction already pushing that transaction. This allows dependency cycles to be discovered. Fixes #20448 25791: jobs: bump default progress log time to 30s r=mjibson a=mjibson The previous code allowed updates to be performed every 1s, which could cause the MVCC row to be very large causing problems with splits. We can update much more slowly by default. In the case of a small backup job, the 5% fraction threshold will allow a speedier update rate. Remove a note that's not useful anymore since the referred function can now only be used in the described safe way. See #25770. Although this change didn't fix that bug, we still think it's a good idea. Release note: None 26293: opt: enable a few distsql logictests r=RaduBerinde a=RaduBerinde - `distsql_indexjoin`: this is only a planning test. Modifying the split points and queries a bit to make the condition more restrictive and make the optimizer choose index joins. There was a single plan that was different, and the difference was minor (the old planner is emitting an unnecessary column). - `distsql_expr`: logic-only test, enabling for opt. - `distsql_scrub`: planning test; opt version commented out for now. Release note: None Co-authored-by: Spencer Kimball <[email protected]> Co-authored-by: Matt Jibson <[email protected]> Co-authored-by: Radu Berinde <[email protected]>
Build succeeded |
The previous code allowed updates to be performed every 1s, which could
cause the MVCC row to be very large causing problems with splits. We
can update much more slowly by default. In the case of a small backup
job, the 5% fraction threshold will allow a speedier update rate.
Remove a note that's not useful anymore since the referred function
can now only be used in the described safe way.
See #25770. Although this change didn't fix that bug, we still think
it's a good idea.
Release note: None