Provide uniform way to handle DBS server errors #11176
Conversation
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
| :return: either (parsed) concise exception message or original exception | ||
| """ | ||
| try: | ||
| data = json.loads(exc) |
There was a problem hiding this comment.
are you sure the dbs3-client returns a json object here?
There was a problem hiding this comment.
the HTTP error body comes from dbs3-client which gets it from DBS server
There was a problem hiding this comment.
My point is, when this response (error) is passed back from the client to our application, is it provided as a JSON object or as a python dictionary?
|
Jenkins results:
|
|
Jenkins results:
|
| :return: either (parsed) concise exception message or original exception | ||
| """ | ||
| try: | ||
| data = json.loads(exc) |
There was a problem hiding this comment.
My point is, when this response (error) is passed back from the client to our application, is it provided as a JSON object or as a python dictionary?
|
And please, fix the PR title. |
vkuznet
changed the title
|
Alan, regarding your #11176 (comment) . I don't have control over clients. But the data-type on a wire is bytes, then clients read them as string. The string may have JSON representation and that's why we read it using |
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
There was a problem hiding this comment.
And it just occurred to me that that line checking for proxy error:
https://github.com/dmwm/WMCore/pull/11176/files#diff-dc5aa561c9a43023153ce0a627108a41a95e7bae0a9b16b474398296f23b92b7R102
is extremely dangerous and we should remove it. Since you are touching this code, could you please remove that elif 'Proxy Error' in exString: as well? Otherwise, I can create another issue and get it fixed as well.
|
Jenkins results:
|
|
BTW, I assume you are no longer working on it. In that case, please remove the "Work in progress" label. @vkuznet |
|
Yet another updated: I just backported it to the wmagent branch, in this PR: #11184 If you find further src/* changes to be applied, please separate that in a new commit because I will then have to cherry-pick that as well (but the best would be to avoid any further changes, if possible). |
| logging.exception(msg) | ||
| logging.debug("block info: %s \n", block) | ||
| results.put({'name': name, 'success': "error", 'error': msg}) | ||
|
|
||
| return | ||
|
|
||
|
|
||
| def parseDBSException(exBodyString): |
There was a problem hiding this comment.
Hi again @vkuznet. Sorry for reiterating through this upon PR approval from my side, but I started wondering if that function won't be better to be a separate method in a dedicated DBSExeption class. One possible place could be here: https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/Services/DBS/DBSErrors.py or equivalent one in WMComponent/DBS3Buffer
Just an idea though, not requesting any changes here. If not accepted, the current approach also works perfectly fine according to me.
There was a problem hiding this comment.
@todor-ivanov , if code is independent (as it is now) it is better to keep it in separated function and not part of any class. But I don't have strong opinion in which module it should reside. So far we use it only in DBSUploadPoller and not anywhere else. As such it is better to reside over there and be treated as local function. But if there is use-case to use it in different parts of WMCore then yes may be DBSErrors.py can be a better place to hold this function .
There was a problem hiding this comment.
Thanks @vkuznet
Agreed :) lets move on as it is right now. And if we find a use case we can always move it in the general exception class.
|
@amaltaro squash is done, and I separated code vs unit test commits. |
|
Jenkins results:
|
|
@amaltaro I'm not sure what happen with jenkins unit tests, all regular tests have failed but the one I added succeed, e.g. I only squashed the changes and I doubt it is an issue. Please guide what to do with this. |
|
test this please |
|
Jenkins results:
|
|
@amaltaro I think with new changes other DBS test may need adjustments. I looked up at failed tests test/python/WMComponent_t/DBS3Buffer_t/DBSUpload_t.py line 620 and from the log it says: |
|
test this please |
|
Jenkins results:
|
|
@vkuznet Valentin, given that the block error handling is embedded in the DBS3Upload Given that we consider I tested with the wmcore container and it seems to do the trick. |
|
Of course we can remove such lines but it means that all unit tests will not test the proxy error. In my view we need to test all possible errors coming from CMSWEB, including proxy error, and the correct approach is to adjust unit tests to not throw the exception but in fact see the proxy error. It is really depends on you view of what unit tests should do. I do not feel that I should make this decision. Please says explicitly which route to proceed. |
|
I said it in my previous comment. Having exhaustive tests for every single exception we might get from CMSWEB would be the ideal, but it's not practical. If you want to make the right thing though, we should have: This is probably much more work than what you have originally planned though, so I am happy with the first suggestion as well (removing those few lines from the emulator). |
|
In this case I suggest to open a different ticket with suggestions you and keep it around. In this one, I commented out Proxy Error mocking. Let's see how jenkins tests will come out. |
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
|
I do not think the |
|
test this please |
|
@amaltaro something really weird in Jenkins, if you look back to unit tests you'll see that each time I run and, in your test it is |
|
Jenkins results:
|
|
Jenkins results:
|
|
Thanks Valentin. The |
Fixes #11167
Status
in development
Description
Provide parser function for DBS Go-based exception. If it successfully parsed the exception message it will return concise message (DBS Go server
reason), otherwise it returns original exception messageIs it backward compatible (if not, which system it affects?)
YES
Related PRs
<If it's a follow up work; or porting a fix from a different branch, please mention them here.>
External dependencies / deployment changes
<Does it require deployment changes? Does it rely on third-party libraries?>