Database Code: Reporting Query Failures

[hamishmb]

Hi all,

So I've almost finished the DatabaseConnection class now I think, but one of the last things left is to figure out how to report query failures. Note that these should not happen in normal operation, so it could be argued that my current approach to handling this is sufficient.

Current (basic) approach to solving the problem

If a query is a write-only operation (for example adding a message to the event log), and the query fails, the pi will disconnect and then reconnect to the database and retry the query. If the query repeatedly fails (which should never happen) this will become an endless loop.

If a query is a read-only operation (for example getting the latest SUMP water level reading), and the query fails, the pi will disconnect and reconnect, and it will *NOT* try to execute that query again, instead signalling failure with a RuntimeError to the code that called the relevant method.

Ideas

• This approach might be just fine as it is, as query errors should not happen anyway.
• If we could figure out why a query failed, we could then take different actions depending on the reason, but it is not clear to me that we can do this.
• If we could report errors for write-only operations, that might be good.
  • However, the corresponding methods do not currently wait for acknowledgement that the write worked - instead it is just guaranteed that eventually it will go through - it will retry forever.
  • ^ There also doesn't seem to be any way to do this as far as I can tell.
  • Any important read request should be repeated shortly anyway - how much does dumping the queries when they fail actually matter?
  • ^ This is in contrast to writes - missing readings or events in the tables could be very bad for us.
• If he could be turned, he could become a powerful ally.

What do you guys think is best? I have no doubt (that this boy is the offspring of Anakin Skywalker) that there are ideas that would work better that I haven't thought of.

[TerryJC]

Hmmm. Ideally, I think we should log all Query errors so that we can investigate later. However, if the methods don't wait (and that is obviously the point with a database), then we would be struggling to trap the errors anyway. In any case, as you say, they shouldn't happen anyway.

Just a thought. Are there any Open Source Database implementations that we could examine to see what they do?

[hamishmb]

Well, the MySQL client library we use is at https://pypi.org/project/mysqlclient/, and it follows the spec PEP-249 (https://www.python.org/dev/peps/pep-0249/) but it seems to be quite light on error handling. It's source code on github is linked from that page.

Maybe that is an idea though. If you think it's worth it I could start looking through the source code.

[TerryJC]

Another thought. Wouldn't the database itself log errors?

[hamishmb]

Good point, I'll have a look at that too. One would hope that it does, but it may depend on D-Link being competent so we shall see XD

[PatrickW]

Sounds good.

It makes sense to me that read errors should result in an exception that can be handled in the code that actually wanted the data.

In the general case, I think it would make sense to do the same for write errors, because the response does depend on the reason for writing the data. However in our specific case I think most of the time the best response for a write error would be to try again. But is it worth implementing the retry behaviour as a wrapper around a version that doesn't retry, so that both are available, rather than making the assumption? (I have no preference, assuming it's even possible.)

If there is any possibility of operations which are both read and write operations, then I think those would need to raise an exception in the calling code, too.

[hamishmb]

Good idea Patrick, then that at least gives us the option to not retry. It would make the control logic functions more complicated though, as they would have to handle it instead.

Alternatively, there could be an extra argument to the relevant DatabaseConnection methods that specifies if you want it to/how many times to retry. This would mean I'd have to use the lock a bit more to ensure thread concurrency doesn't cause problems, but that's really no trouble.

I had a look and it turned out that it is possible to read and write in a single query, but none of our need to do that, and I would think we'd be best off keeping it simple and using separate queries.

[PatrickW]

Ordinarily the retrying version would be used, so the complexity of the control logic need not increase. Definitely still provide the retrying version.

My instinct says two methods are better than one method with a parameter that changes its error-handling behaviour, but I'll leave that up to you.

Unless the non-retry version would basically just be a call to the underlying database library, in which case I've underestimated the level of abstraction again, like I did with "Sockets".

[hamishmb]

The non-retry version would essentially just be a call to the underlying library.

How about an optional argument to set the number of retries (including 0), with a default of 5 retries (if unspecified) before giving up and throwing an error?

[PatrickW]

Oh, well in that case I am inclined to go along with your original suggestion of retrying "forever". Maybe there's a reason to allow a different number of retries, but I can't see it.

The reason I was suggesting a non-retry version was because it would be a necessary building block for other behaviours if they turned out to be useful in the future. Given that there is a non-retrying version in the underlying library, a version that can do different numbers of retries doesn't really add anything in terms of building blocks, it's just extra features without justification.

With the 'retry forever' version, I was assuming it would be an asynchronous operation; the control logic just says "write this" and then moves on. Otherwise, you might be avoiding the loss of that one write, but completely preventing several subsequent ones which were just as important. But the limited retries version presumably has to be synchronous (the control logic is going to want to wait to see the outcome and deal with the possible error). So, I would think they need to be two separate methods.

I suppose you could write one method that does both, by having the caller provide a callback to deal with the errors, but then any code that calls the 'forever' version has to specify a callback that never does anything. And using a callback means the error handling for the not-forever version has to run concurrently. Which is all making my head spin.