Properly parse and handle DBS client errors #12312
Conversation
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
There was a problem hiding this comment.
If I understand correctly, these changes make sure that when we fail to insert a block in dbs, we keep track in the agent of all the errors that have been encountered inside dbs, to make sure that we always have the root cause for the error on our side.
If this is the case, then i approve, looks good to me
There was a problem hiding this comment.
Before going with a full review, I'd rather have those commits properly squashed first - as every now and then I observe changes getting lost during the squashing process.
In addition, I think it is important to first move with the server changes (dbs2go). Once testbed is running the new dbs2go version, please test these changes in an agent with a representative use scenario.
Lastly, I think our agreement was that either error.reason or message would have a clear and friendly error message (with the actual root error). Did I misunderstand our discussion? Why do we have to have string parsing of the error served by the server?
vkuznet
force-pushed
the
fix-issue-12229
branch
from
3ebc1a0 to
c7112c7
Compare
|
|
Jenkins results:
|
|
@amaltaro please note that reported unit test failures are not related to this PR changes. |
|
hi @vkuznet :
I am testing it now, before merging. |
|
test this please |
|
Jenkins results:
|
There was a problem hiding this comment.
As mentioned in my previous review, I think we should not be traversing all the errors from the server in our client layer, instead we should use the final and root cause error that the server will be providing. I also left a comment along the code.
Before we converge and merge this PR, I think we need to:
a) upgrade dbs2go in testbed
b) update this PR to use the actual precise error (message or reason field?)
c) apply the candidate PR/patch to an agent
d) test this with a representative scenario.
| msg = f"Error trying to process block {name} through DBS. Details: {msg}" | ||
| logging.error(msg) | ||
| results.put({'name': name, 'success': "error", 'error': msg}) | ||
| for srvCode in dbsError.getCodes(): |
There was a problem hiding this comment.
Given what I reported/asked in the main PR body, I think looping over the errors is not what we have discussed. Instead, the agent should only care about the actual root cause of the injection failure, without traversing the whole chain of errors.
There was a problem hiding this comment.
@amaltaro , I suppose this comment is outdated after the MM discussion, right?
| @@ -96,6 +96,17 @@ def getServerCode(self): | |||
| return self.data['error']['code'] | |||
| return 0 | |||
|
|
|||
| def getCodes(self): | |||
| """ | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I do not get the comment here.... @amaltaro, what is not followed by the book in this docstring ?
There was a problem hiding this comment.
"Parse the response of DBS Server and return all the error codes from error.code" or something providing a brief summary
There was a problem hiding this comment.
I would leave the description as is, given that the function is not parsing and then separately putting the results in a list, but it's creating a list by making the associations between the error messages and the error codes
There was a problem hiding this comment.
I think the implementation of the function/method does not influence the structure of the docstring.
But if people want to update the contribution guidelines with a different recommendation, let us do it.
I am just trying to seek consistency in the project.
|
Jenkins results:
|
|
I addressed @amaltaro concern about "friendly" messages via the following PR: dmwm/dbs2go#129 In particular, I modified 520 (!!!) errors in DBS server code and move away from generic errors to concrete ones which address friendliness of returned DBS error code error and messages. I'll provide additional changes in this PR to address backward compatibility and avoid explicit reliance on DBS error code values. Stay tuned. |
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
Fixes #12229
Status
ready
Description
Properly collect all DBS error codes and provide new method to represent them to the client. The client code can request for DBS error codes and interpret them accordingly. Adjust DBSUploadPoller to use DBS error codes and perform necessary logic to spit out proper error message to upstream caller.
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
After numerous discussions via MatterMost here is a list of all related PRs:
External dependencies / deployment changes