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

Remote Backend: Throw DBException in functions that allow it #8943

Merged
merged 3 commits into from
Jun 5, 2020

Conversation

pieterlexis
Copy link
Contributor

Short description

These functions are called inside a try block in e.g. the
slavecommunicator and them throwing actually makes the reporting
better.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@pieterlexis pieterlexis requested a review from Habbie March 17, 2020 16:23
@rgacogne
Copy link
Member

Out if curiosity, did you consider making RemoteBackend::send() and RemoteBackend::recv() throw instead? It might solve #8866 as well.

@Habbie Habbie added this to the auth-4.3.x milestone Mar 18, 2020
@pieterlexis
Copy link
Contributor Author

Out if curiosity, did you consider making RemoteBackend::send() and RemoteBackend::recv() throw instead? It might solve #8866 as well.

I was afraid with blowing up the auth process on a failure.. but it looks like the database backend do the same as well. I will change this PR to indeed throw on send and recv failures.

@pieterlexis
Copy link
Contributor Author

hhrm, by the looks of the CI, throwing on {"result": false} might be a bit too much :)

@rgacogne
Copy link
Member

hhrm, by the looks of the CI, throwing on {"result": false} might be a bit too much :)

That's a valid result for some operations, I believe?

@pieterlexis
Copy link
Contributor Author

That's a valid result for some operations, I believe?

it is

@pieterlexis pieterlexis force-pushed the remote-commit-false branch 3 times, most recently from 959baf1 to ba35393 Compare March 19, 2020 09:27
@pieterlexis pieterlexis force-pushed the remote-commit-false branch 3 times, most recently from 3a6a5b3 to e3dbf3c Compare April 1, 2020 13:00
These might indicate that the function is not implemented
// bad.
g_log<<Logger::Warning<<"Received unacceptable HTTP status code "<<std::to_string(resp.status)<<" from HTTP endpoint"<<endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be DBException instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I typed that in, but I think blowing up the connection on a 404 is a bit much.....

Copy link
Contributor

Choose a reason for hiding this comment

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

uhm, the if condition precludes 404, so it's not blowing up the connection.. I am bit puzzled how 404's are treated now.

Copy link
Contributor

Choose a reason for hiding this comment

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

should there be unit/integration test for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhm, the if condition precludes 404, so it's not blowing up the connection.. I am bit puzzled how 404's are treated now.

ah yes :)

should there be unit/integration test for that?

It is implicitly tested, as I got exceptions on 404s before :)

Copy link
Member

Choose a reason for hiding this comment

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

This looks resolved to me, except for the exception type, right?

@Habbie
Copy link
Member

Habbie commented Jun 5, 2020

Title says DBException, code says PDNSException, I trust the title is outdated?

@pieterlexis
Copy link
Contributor Author

Title says DBException, code says PDNSException, I trust the title is outdated?

We use PDNSExceptions to signal problems, when they aren't recoverable by the backend, we throw a DBException toward the Ueberbackend.

@Habbie
Copy link
Member

Habbie commented Jun 5, 2020

We use PDNSExceptions to signal problems, when they aren't recoverable by the backend, we throw a DBException toward the Ueberbackend.

Oh, I see now that the code does both. Ok!

@Habbie Habbie merged commit b4a587a into PowerDNS:master Jun 5, 2020
@pieterlexis pieterlexis deleted the remote-commit-false branch June 5, 2020 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants