fix(cli) prefix Cassandra connector errors in new DAO #3648
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thibaultcha
force-pushed
the
fix/db-prefix-err
branch
from
538bf78 to
d20193e
Compare
thibaultcha
changed the title
Tieske
reviewed
Jul 26, 2018
| return nil, prefix_err(self, err) | ||
| end | ||
|
|
||
| return ok |
There was a problem hiding this comment.
considering the repetitive nature of this error checking, could we do something like:
local function prefix_err(self, ok, err, ...)
if not ok then
return ok, "[" .. self.strategy .. " error] " .. err
end
return ok, err, ...
end
-- and calling it here (and in other places) as:
return prefix_err(self, self.connector:truncate())can probably be cleaner if exporting prefix_err as well
There was a problem hiding this comment.
Considered it, but I don't want to introduce the extra cost of calling a variadic function on every call of the connector's functions. Five occurrences, and it is unlikely that we add more methods to the connector, is not that repetitive is what I told myself.
thibaultcha
force-pushed
the
fix/db-prefix-err
branch
from
d20193e to
3e3c622
Compare
Since recently, we call the new DAO's `assert(db:init_connector())` in the CLI _before_ instantiating the old DAO (because the latter needs the former as an attribute). For Cassandra, this has the effect of retrieving the cluster's topology, and thus trying to connect to the given contact points. If any error is encountered, the new DAO does not properly prefix the error with the `[Cassandra error]` prefix, as the old DAO would do. This goes unnoticed for the PostgreSQL strategy since the connector initialization is nopping. See a related CI failure here: https://travis-ci.com/Kong/kong-private/jobs/136391637#L1094 We now prefix connector errors (which are always strings).
thibaultcha
force-pushed
the
fix/db-prefix-err
branch
from
3e3c622 to
dc9857e
Compare
kikito
approved these changes
Jul 27, 2018
hutchic
approved these changes
Jul 27, 2018
hishamhm
reviewed
Jul 27, 2018
| @@ -81,6 +83,7 @@ function DB.new(kong_config, strategy) | |||
| daos = daos, -- each of those has the connector singleton | |||
| strategies = strategies, | |||
| connector = connector, | |||
| strategy = strategy, | |||
There was a problem hiding this comment.
I had the exact same change done in my codebase here as I work on the Plugins entity. :) We really need to start making smaller PRs with this little "minor tweaks" as we work through major changes!
|
Thank you all for taking a look |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since recently, we call the new DAO's
assert(db:init_connector())inthe CLI before instantiating the old DAO (because the latter needs the
former as an attribute).
For Cassandra, this has the effect of retrieving the cluster's topology,
and thus trying to connect to the given contact points. If any error is
encountered, the new DAO does not properly prefix the error with the
[Cassandra error]prefix, as the old DAO would do. This goes unnoticedfor the PostgreSQL strategy since the connector initialization is
nopping.
See a related CI failure here:
https://travis-ci.com/Kong/kong-private/jobs/136391637#L1094
We now prefix connector errors (which are always strings). We also include the strategy in the reported error, as it should have always been, but was missing due to the lack of second argument (it is optional) when calling the
DB.new()function from the tests.