Skip to content

ffldb: Add dbErr to error description.#2876

Merged
davecgh merged 2 commits intodecred:masterfrom
JoeGruffins:moreerrorinfo
Feb 8, 2022
Merged

ffldb: Add dbErr to error description.#2876
davecgh merged 2 commits intodecred:masterfrom
JoeGruffins:moreerrorinfo

Conversation

@JoeGruffins
Copy link
Copy Markdown
Member

@JoeGruffins JoeGruffins commented Jan 26, 2022

ffldb: Add dbErr to error description.

Add the reason for bad inserts, updates, and deletes to the error
description and display key as hex in order to give the user more
information when the database errors. This is especially useful for
upgrades which may fail for several reasons, such as low disc space,
which the user will now be informed about.

blockchain: Add ldbErr to error description.

Add the reason for bad inserts, updates, and deletes to the error
description.

I am sorry for not making an issue first. I believe displaying the key as hex was lightly discussed in matrix and the consensus was that it's ok.

Error for low disc space on master:

[ERR] DCRD: Unable to start server: failed to put key "\x00\x00\x00\x19\x1dn\xf1\x14()d\x17*\xa2\x91y\x9c\xcc\xf5Ic{(\xbaW\xbc\xe3\x92G\x00\x00\x00\x00\x00\x00\x00" to ldb transaction

With this diff:

[ERR] DCRD: Unable to start server: failed to put key 0000001620a9ba256242f74a82afe270bacaae88680e636b698cca830500000000000000 to ldb transaction: write /home/joe/.dcrd/data/mainnet/blocks_ffldb/metadata/069617.ldb: no space left on device

Tested with a real upgrade.

Copy link
Copy Markdown
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good improvement to me. The underlying error is being stored in the RawErr field of the converted error, but that field is never displayed in the error output.

Regarding displaying the key as hex, we did have a brief discussion of it in this PR for a similar error case.

If you are updating this location, it probably makes sense to update the case below for deletions as well. Or, another option could be to update the convertErr function to include the error to handle it for all cases.

@JoeGruffins JoeGruffins changed the title dbcache: Add dbErr to error description. ffldb: Add dbErr to error description. Jan 27, 2022
@JoeGruffins
Copy link
Copy Markdown
Member Author

Now returning more info for all errors. Unsure how to trigger others besides low disc space, but that one looks the same.

@rstaudt2
Copy link
Copy Markdown
Member

Now returning more info for all errors. Unsure how to trigger others besides low disc space, but that one looks the same.

The update looks good to me. Just a couple of minor comments:

  • It would be good to make the same update at this location for the UTXO backend as well (could be the same or separate commit)
  • It is probably worth updating the function comment to indicate that the original error is included in the description so that the caller is aware

Add the reason for bad inserts, updates, and deletes to the error
description and display key as hex in order to give the user more
information when the database errors. This is especially useful for
upgrades which may fail for several reasons, such as low disc space,
which the user will now be informed about.
@JoeGruffins
Copy link
Copy Markdown
Member Author

doh! I shoulda run tests.

I realized this caused some of the errors to print twice, as it was using the error's Error for the description. Gave those different descriptions that say a little more about where the error took place. Will fix tests.

Add the reason for bad inserts, updates, and deletes to the error
description.
Copy link
Copy Markdown
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested both locations by updating the leveldb code to force an error to be returned, and it worked as expected.

Copy link
Copy Markdown
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good and I also tested various failures by intentionally inducing them. Showing the underlying error is nice as it can often make it more obvious why something failed in cases such as out of disk space errors.

@davecgh davecgh merged commit 1f187c2 into decred:master Feb 8, 2022
@davecgh davecgh added this to the 1.8.0 milestone Feb 8, 2022
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.

4 participants