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

Swap NotMasterError with NotPrimaryError. #11016

Merged

Conversation

todor-ivanov
Copy link
Contributor

@todor-ivanov todor-ivanov commented Mar 1, 2022

Fixes #11012

Status

Ready

Description

The current PR is intended to handle the newly introduced exception NotPrimaryError on the pymongo client side, which is about to substitute the old one NotMasterError, starting with pymongo version 4.01 . We have tested and proved that on the server side the latest version is already supporting both the old and the new clients, so no errors in exception handling are originating from client vs server version mismatch.

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

NO

Related PRs

There is about to be a cmsdist related PR to upgrade the pymongo rpm package:
cms-sw/cmsdist#7654

External dependencies / deployment changes

Yes.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 7 tests deleted
    • 3 changes in unstable tests
  • 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/12831/artifact/artifacts/PullRequestReport.html

Fix a depricated interface client.database_names() && Rename NotPrimaryError at MSUnmerged.py
@todor-ivanov todor-ivanov force-pushed the bugfix_MSUnmerged_NotMaterError_fix-11012 branch from 3d6c25d to 19b41a9 Compare March 1, 2022 15:06
@todor-ivanov todor-ivanov requested review from amaltaro and vkuznet March 1, 2022 15:15
@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 7 tests deleted
  • 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/12832/artifact/artifacts/PullRequestReport.html

@amaltaro
Copy link
Contributor

amaltaro commented Mar 1, 2022

Todor, don't we have to update any of the exception handling in our baseline source code? I had the impression it required a pymongo bump and a WMCore change as well.

@todor-ivanov
Copy link
Contributor Author

todor-ivanov commented Mar 1, 2022

Hi @amaltaro This
https://github.com/dmwm/WMCore/pull/11016/files#diff-cd601ffcba537b986d36fc945d0d34012052b4c198c3fbd14335e01579849b85R141

is the only strictly WMCore related change, which may affect other services using Mongodb. It is related to a change of a deprecated method in pymongo 4.0.1. But this one is there since a while now (with a deprecation warning). The new method (client.list_database_names()) exists even in pymongo 3.10.1, see [1]. So this change is not going to break anything in other services using pymongo. The rest of the code changes affect only MSUnmerged micro service.

[1]

In [2]: msUnmergedDB.command({'buildInfo':1})['version']
Out[2]: '5.0.3'

In [3]: import pymongo

In [4]: pymongo.version
Out[4]: '3.10.1'

In [5]: msUnmergedDB.client.database_names()
~/WMCore.venv3/bin/ipython:1: DeprecationWarning: database_names is deprecated. Use list_database_names instead.
  #!~/WMCore.venv3/bin/python3
Out[5]: ['admin', 'config', 'local', 'msUnmergedDBPreProd', 'msUnmergedDBProd']

In [6]: msUnmergedDB.client.list_database_names()
Out[6]: ['admin', 'config', 'local', 'msUnmergedDBPreProd', 'msUnmergedDBProd']

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.

Todor, it looks like I forgot to scroll down and missed those baseline changes.
While these changes look good to me, I would say it cannot be merged until we cover all the 3 MongoDB use cases that we have in WMCore (as mentioned a minute ago in the GH issue itself).

@todor-ivanov
Copy link
Contributor Author

Thank you @vkuznet !

BTW I think I have somehow closed that issue by mistake. Reopening it with the current comment.

@todor-ivanov todor-ivanov reopened this Mar 2, 2022
@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 7 tests deleted
  • 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/12835/artifact/artifacts/PullRequestReport.html

@amaltaro amaltaro self-requested a review March 3, 2022 20:33
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.

Just making sure my review - and discussion - gets posted to the PR instead of the issue itself: #11012 (comment)

I think it would be important to have a fully functional change. And given that it was mentioned it would be a near future issue, we might want to decide to come back to this in the very near future as well (hopefully with the VM setup no longer valid).

@amaltaro
Copy link
Contributor

amaltaro commented Mar 4, 2022

Okay, I tried to upgrade MongoDB in COMP, but it's not that straight forward.

Let's get this merged then, but before that, we should check why those unit tests are reported as deleted (I think we need to first upgrade pymongo version, I will get that cmsdist PR merged and then we can come back to this later today or tomorrow).

@amaltaro
Copy link
Contributor

amaltaro commented Mar 4, 2022

test this please

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 1 changes in unstable tests
  • 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/12846/artifact/artifacts/PullRequestReport.html

@amaltaro
Copy link
Contributor

amaltaro commented Mar 5, 2022

This looks better now.

Note that this will likely break our MSOutput and MSUnmerged services when running on RPM-based MongoDB setup. @todor-ivanov can you please update the initial description with this note as well? Thanks

@amaltaro amaltaro merged commit 6a78d41 into dmwm:master Mar 5, 2022
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.

MSUnmerged: NotMasterError renamed to NotPrimaryError
4 participants