Skip to content
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

Check data-type of fileLumisList #73

Merged
merged 2 commits into from
Aug 30, 2022
Merged

Conversation

vkuznet
Copy link
Contributor

@vkuznet vkuznet commented Aug 26, 2022

In order to extract element from the dict we need to ensure that provided object is indeed a dict data-type. This PR provides this fix for issue dmwm/CRABServer#7378

@vkuznet
Copy link
Contributor Author

vkuznet commented Aug 26, 2022

@amaltaro , @mapellidario please review the change I applied to dbsClient to check data type of the object. If you'll sign off this change I'll push it forward and create new dbsClient tag and upload it to PyPi

@amaltaro
Copy link
Contributor

It's hard to review it when you do not know the expected data structure, but I managed to find a dump from 2019, where we can see that this file_lumi_list is supposed to be a list of dictionaries:
https://amaltaro.web.cern.ch/amaltaro/forYuyi/dbsuploader_block.json

I am reviewing your changes now.

Copy link
Member

@mapellidario mapellidario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me. Thanks Valentin for providing the fix promptly! :)

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valentin, these changes look good to me. But I suggested possible improvements to the code which is up to you to decide.

Before you make a new release, maybe @mapellidario could patch TaskPublisher - if possible - and test your final changes here before pushing this all the way upstream/services.

@@ -607,8 +607,12 @@ def insertBulkBlock(self, blockDump):
if len(blockDump['files']) > 0:
fileLumiList = blockDump['files'][0]['file_lumi_list']
if len(fileLumiList) > 0:
if fileLumiList.get('event_count') == None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we know it's a list of dictionaries, I think you could replace most of this code with something safer, e.g.:

flDict = next(iter(fileLumiList), {})
if flDict.get('event_count') is None:
    frst = False

It might be better to rename frst to hasEventCount, or something like that as well.

@mapellidario
Copy link
Member

Sure, we can test this in testbed before merging

@vkuznet
Copy link
Contributor Author

vkuznet commented Aug 26, 2022

@amaltaro I think we should keep code as is (with changes I provided) as it suppose to work with both DBS server implementations. As we gradually move from one to another I doubt it would be wise to change code to satisfy one server implementation details as we may (even with a small probability) to rollback DBS server and it would be harder to go back with changes. Therefore are some parts which will need clean-up but this step was post-poned until we perform full migration to new implementation.

@vkuznet
Copy link
Contributor Author

vkuznet commented Aug 29, 2022 via email

@mapellidario
Copy link
Member

While having a better look, I noticed that in your changes you sometimes use fileLumiList and fileLumisList.

Assuming that it is a typo, I changed the code to only use fileLumiList and it works smoothly. thanks Valentin!

@vkuznet
Copy link
Contributor Author

vkuznet commented Aug 30, 2022

@mapellidario, thanks for spotting a typo. I fixed it now and will proceed with release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants