-
Notifications
You must be signed in to change notification settings - Fork 107
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
Implement support for gzipped stream for RucioConMon #11142
Conversation
@amaltaro , @todor-ivanov it is initial implementation and I'm seeking your feedback. To resolve the issue I fixed Services class which handle all HTTP requests, in particular I added |
Jenkins results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vkuznet I am not sure it would work for every single case, but the idea that came to my mind was to actually define this binary
mode in the super class (Service) at the __init__
method. Sub-classes inheriting from it - like RucioConMon - would then define one more option in configDict
and pass it over, identifying that its http responses are meant to be dealt with in a binary mode.
I do not know though how to handle a class that we would be fetching human-readable responses and binary at the same time. Perhaps trying to read it as binary first and having a fallback (try/except) to human-text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vkuznet that is great you did provide the implementation of this method.
I had only one minor comment inline. Please take a look. The fix should be trivial.
Thank you.
Alan, I'm not sure which use-cases you're referring to, but I preserved the existing ones, and obviously these use-cases are unable to read binary data which I fixed in this PR. Unless you specify all use-cases you envision I don't think we can have any productive discussion on how code should be structured. I also don't think the Service has anything to do with data-format. If you want to have generic behavior you need to introduce intermediate object which will deal with different data-types. For instance, |
Jenkins results:
|
Valentin, what I was proposing is an alternative to the way this So, the whole logic is basically the same as the one you implemented, the only different is on how you notify the superclass (Services) that you want to deal with binary http requests or not. Given that the I am trying to generalize and make it easier to be adopted in other parts of the code. However, as previously mentioned, it could be that for a given class object we want only part of the calls to be dealt with in a binary mode, and this is what is not clear to me whether we can and how we could solve it. |
Alan, thanks for clarification, it is impossible to do that in Service class since it does both things, i.e. the data retrieval and cache management. But data retrieval part depends on upstream service, i.e. some services, e.g. DBS/RucioConMon can provide gzipped content, while other, e.g. CRIC, WMStats do not. But since caching part is part of Service and uses I suggest that you make your mind and tell me how you'd like to see it. My intention was only to adjust existing code without major refactoring. As such, I think adding |
@vkuznet Valentin, I remember we had a very productive discussion on this topic in the past week, but I no longer remember all the details. I think we agreed that the best would be to use the http headers (from the response object) as a generic feature. While for RucioConMon, we use the headers but we also keep request |
@amaltaro , yes it is correct conclusion. |
@amaltaro , let's not forget about this PR, the code is ready and again it is now in the air of awaiting something which I have no control. What is the conclusion how to move forward with this? |
@vkuznet in that case, the conclusion is that we are not going to add another keyword to those methods in the super class |
@amaltaro I'm more interested what we should do with this PR, it is still unclear from your answer. Please clarify if any action should be taken to move forward with the PR itself. |
@vkuznet Valentin, sorry for not being specific with my previous answer. IMO, we should:
If it's still not clear and you prefer to have a chat about this, then we can talk in the beginning of the next week. |
Alan, it seems to me that you're mixing different things. The binary option to the superclass is mandatory to write binary content. This content you may get either specifying HTTP header or using HTTP end-point flags (as RucioConMon does). The data arrive in binary format and if you want to write it to the cache you must open file with binary flag for read/write operations. Let's setup chat meeting about it to resolve misunderstandings. |
test this please |
Jenkins results:
|
test this please |
Jenkins results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vkuznet I had another look at this implementation and left a few comments for your consideration. Thanks
Jenkins results:
|
Jenkins results:
|
Jenkins results:
|
Jenkins results:
|
Jenkins results:
|
Jenkins results:
|
Alan, now all test/pylint are clear. Feel free to review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valentin, it's looking good to me. Since you still have to squash these commits, I left another minor enhancement comment.
Now that you have these final changes, could you please test your integration unit test as well, to make sure that everything is behaving? Thanks
Jenkins results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vkuznet
This look good to me.
@vkuznet once you confirm the unit test is still succeeding, this is ready to go. |
Here is results from integration test:
I hope it is convincing and you can merge this PR :) |
Great! You forgot squashing those commits though (keep in mind to keep test in a separate commit please) and remove those labels, if it's ready to go from your side. @vkuznet |
Alan, I squashed all commits, but test file was added as part of initial commit which cannot be separated. |
Jenkins results:
|
Alan, the new failed unit test after the squash is related to CouchDB, see here
I have no idea how this is possible, but I really doubt it is related to squash of commits. |
Thanks for looking into the details of the unit test failure. It's a known issue and meant to be tracked in: Regarding unit tests, please put them in separate commits for future PRs. |
Fixes #11036
Status
ready
Description
Implement support for zipped I/O while calling RucioConMon service. The changes required to introduce binary read/write mode operation on cache and properly read and write files in binary mode.
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
It is similar in nature to #11041 (since it relies on
gzip
library) but it adjust WMCore service to properly handle binary content in its cache.External dependencies / deployment changes
None