Skip to content
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

mysql: check for error when getting subtrees #3173

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Oct 31, 2023

treeTX.getSubtrees iterates over a set of rows from database/sql, but it was failing to check the error status after exiting the for loop that iterates over them.

On our log deployment (Let's Encrypt Oak), this is causing mistaken error messages of preload did not get all tiles when the real error should be propagated from mysql (in our case, max_statement_time exceeded).

We're in the middle of trying to remediate a significant availability from for get-sth-consistency, and getting this landed and deployed would help eliminate a source of noise in our investigation.

@jsha jsha requested a review from a team as a code owner October 31, 2023 00:11
@jsha jsha requested a review from jiggoha October 31, 2023 00:11
@roger2hk
Copy link
Contributor

/gcbrun

@mhutchinson
Copy link
Contributor

@jsha can you rebase this on top of latest master and re-push? cloudbuild is failing some bazel checks that haven't been in this repo for some time. Your parent commit is currently:

commit ec18a4bc2732bd21b1d40ae6d62cdbe6466ffccf
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Wed Dec 8 09:32:59 2021 +0000

treeTX.getSubtrees iterates over a set of rows from `database/sql`, but
it was failing to check the error status after exiting the for loop that
iterates over them.

On our log deployment, this is causing mistaken error messages of
`preload did not get all tiles` when the real error should be propagated
from mysql (in our case, max_statement_time exceeded).
@jsha
Copy link
Contributor Author

jsha commented Oct 31, 2023

Done, thanks!

@mhutchinson
Copy link
Contributor

/gcbrun

@mhutchinson mhutchinson merged commit 8d2d17a into google:master Oct 31, 2023
9 checks passed
@jsha jsha deleted the close-those-rows branch October 31, 2023 17:04
@jsha
Copy link
Contributor Author

jsha commented Oct 31, 2023

Thanks!

roger2hk added a commit to roger2hk/trillian that referenced this pull request Nov 1, 2023
roger2hk added a commit that referenced this pull request Nov 1, 2023
* Update CHANGELOG.md for v1.5.3 release

* Add /pull/3173 in CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants