DBS3Upload: check error message from the response body attribute

[amaltaro]

Fixes #10960

Status

ready

Description

It isn't clear why we have this new behaviour now, but it could be from the new version of dbs3-client/pycurl.

When looking up at the error message, use the body attribute of the HTTP response object, which would have something like:

{"exception": 400, "message": "DBSBlockInsert/insertBlock. Block /RelValProdMinBias/Agent157_Val-Agent157_Val_OLD_Alanv3-v22/GEN-SIM#d6eb571b-f65e-4fb3-af49-ceb16b55a14e already exists.", "type": "HTTPError"}

different than the stringyfication of the exception object, which was giving us only this message:

400: Bad Request

Is it backward compatible (if not, which system it affects?)

YES

Related PRs

None

External dependencies / deployment changes

None

[cmsdmwmbot]

Jenkins results:

• Python2 Unit tests: succeeded
• Python3 Unit tests: succeeded
  • 1 changes in unstable tests
• Python2 Pylint check: succeeded
  • 6 warnings
  • 28 comments to review
• Python3 Pylint check: succeeded
• Pylint py3k check: succeeded
• Pycodestyle check: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/12731/artifact/artifacts/PullRequestReport.html

[amaltaro]

This patch has been already tested in testbed (WMA 1.5.7) and it works like a charm.

[vkuznet]

Even though the change is trivial I doubt it is correct. You should never rely on body to treat the errors. The typical example is this. In DBS Python server the exceptions send as dicts. In Go Server all results, including errors are send as lists. Moreover, the server should be free to change its error representation at any time which implies that client should be able to treat errors correctly. The Body part is only for information and information can be changed over time. Therefore, the code may break depending how you read the body, e.g. if exception contains word "body" or not (for example in Go there is no concept of exceptions even though I followed DBS Python way to create error messages). The more reliable way to check for errors is to read HTTP code instead of the body. Even though I'll approve this change I suggest to pay attention to such exception handling. I understand that it may require lots of changes in different places to use HTTP code instead of body, but this is correct way to do it.

[amaltaro]

Thanks for this review, Valentin. The big problem here is that HTTP error code is 400, nothing else. And this is the same error code that we would get for different problems.
What I understand from your message is that, in order to have a solid fix for this, we should agree on a set of error codes that can be returned from the code, and adapt our applications for that. Is that correct? If so, I fully support it, but as you said, it's probably something for the future.

[vkuznet]

@amaltaro , yes, we should use different HTTP error codes, but their set is limited. You can easily see all of them here. What probably should be done is standardization of DBS error codes and DBS error message(s). For instance, each DBS error message should have:

• DBS error code
• DBS message
• DBS api
  etc.

With such convention we can come up with standard parser on a client side such that application will know exactly what each error message is and how to parse it. I already have some structure in new DBS server and it can be improved further once we made a decision about standard error format/codes.

[amaltaro]

Ok, I suggest we follow this up on this issue that I just created: #10962
I do not think we have room to deal with it this quarter though, so I have already pre-assigned it for the next quarter.

[todor-ivanov]

Thanks @amaltaro
The change indeed looks trivial and I have nothing more to add other than what has already been discussed with Valentin.