Make database failures in LeveldbValueStore more descriptive.

Make database failures in LeveldbValueStore more descriptive.

Note that I initially intentionally didn't do this so that extensions didn't
get any unnecessary information. However, it will help with bug reports.


BUG=149888
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157713

[not at google - send to devlin, 2012-09-17 06:38:40]

Not 100% sure I like it, but WDYT? Should I try to add some UMA for this?

[not at google - send to devlin, 2012-09-17 06:39:25]

Btw +dgrogran in case you have any insight into things that might not be
failures, or might be recoverable, or something.

[Matt Perry, 2012-09-17 19:21:35]

LGTM.

https://codereview.chromium.org/10928227/diff/2001/chrome/browser/value_store...
File chrome/browser/value_store/leveldb_value_store.cc (right):

https://codereview.chromium.org/10928227/diff/2001/chrome/browser/value_store...
chrome/browser/value_store/leveldb_value_store.cc:240: return
WriteFailureForKey("set multi (read)", it.key(), read_error);
since this is a user-visible error, I would make it less cryptic. maybe "set
multiple values (read)". though I don't understand how this is a read operation.

[dgrogan, 2012-09-17 19:46:20]

It's been a while since I looked at leveldb failures, but what you have seems
reasonable. jsbell@ has looked at them more recently, though that was also a
while back.

[not at google - send to devlin, 2012-09-17 22:17:36]

Thanks David. I added an inline comment with details. I'll pick this up either
today or tomorrow (depends how far I get with that other thing).

https://codereview.chromium.org/10928227/diff/2001/chrome/browser/value_store...
File chrome/browser/value_store/leveldb_value_store.cc (right):

https://codereview.chromium.org/10928227/diff/2001/chrome/browser/value_store...
chrome/browser/value_store/leveldb_value_store.cc:378: NOTREACHED() <<
"IsNotFound() but writing?!";
Here (and a few other places checking IsNotFound) are actual functional-ish
changes... but as the NOTREACHED implies, how writing (or clearing, or getting
from a known set) could trigger NotFound is puzzling. I even tried to go through
the leveldb source and couldn't find it.

The other reasons for failures are:
- Corruption
- NotSupported
- InvalidArgument
- IOError

which all seems like legitimate errors (perhaps I'm doing something NotSupported
somehow?)

[dgrogan, 2012-09-19 21:05:47]

lgtm

We'd be curious to know what you find with this.

[not at google - send to devlin, 2012-09-20 00:11:53]

https://codereview.chromium.org/10928227/diff/2001/chrome/browser/value_store...
File chrome/browser/value_store/leveldb_value_store.cc (right):

https://codereview.chromium.org/10928227/diff/2001/chrome/browser/value_store...
chrome/browser/value_store/leveldb_value_store.cc:240: return
WriteFailureForKey("set multi (read)", it.key(), read_error);
On 2012/09/17 19:21:35, Matt Perry wrote:
> since this is a user-visible error, I would make it less cryptic. maybe "set
> multiple values (read)". though I don't understand how this is a read
operation.

Done.

The "read" is because it's the reading part of setting (finding the old value)
versus the writing part (writing the data). I just need something to distinguish
them.

[commit-bot: I haz the power, 2012-09-20 00:12:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10928227/10001

[commit-bot: I haz the power, 2012-09-20 00:34:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-20 02:50:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10928227/14002

[commit-bot: I haz the power, 2012-09-20 05:58:25]

Change committed as 157713