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

[CouchDB] Remove CouchDB all_or_nothing option for bulk API #11011

Merged
merged 2 commits into from
May 20, 2022

Conversation

amaltaro
Copy link
Contributor

@amaltaro amaltaro commented Feb 25, 2022

Fixes #11009
Superseeds #10780

Status

ready

Description

CouchDB all_or_nothing option is no longer available for bulk APIs. This PR stops using that option.
Note that if there is a conflict with one or more of the documents being updated, the conflict is only resolved if a callback function is passed over.

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

YES

Related PRs

Superseeds #10780
by adding a few extra checks in the tests and other minor improvements.

Many other changes have been provided in this PR: #11001

External dependencies / deployment changes

Changes required for CouchDB 3.x, but can also be merged while we are at CouchDB 1.6.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 5 tests no longer failing
    • 2 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/12828/artifact/artifacts/PullRequestReport.html

Comment on lines 165 to 168
if unit._couch.documentExists(unit.id):
self.logger.info('Element "%s" already exists, skip insertion.' % unit.id)
continue

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @amaltaro I seem to fail grasping the need of reordering of these checks (The one above and the if parent one below.) Could you please shed some light?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this check up, because when the document already exists, we do not (re-)insert it into CouchDB, neither we return this document/object back from this method. So, it looks to me the sooner we check and skip this document, the better.
Of course, it could be a problem if this list of documents is still used later in the chain somehow, which I am not 100% sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I am going to revert it just to reduce the amount of changes required for the couch migration. Thanks Todor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Alan! Just to note, I was not requesting a change to it, I was asking more like... for me to understand it.

Copy link
Contributor

@todor-ivanov todor-ivanov left a comment

Choose a reason for hiding this comment

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

Thanks @amaltaro The change looks good to me. I had only one comment inline. It is not requesting any change though, it is just for clarification purposes.

@cmsdmwmbot
Copy link

Jenkins results:

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

@amaltaro amaltaro changed the title Remove CouchDB all_or_nothing option for bulk API [CouchDB] Remove CouchDB all_or_nothing option for bulk API Mar 17, 2022
Copy link
Contributor

@todor-ivanov todor-ivanov left a comment

Choose a reason for hiding this comment

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

Thanks @amaltaro I had only one question inline. But the changes look good to me.

Copy link
Contributor

@vkuznet vkuznet left a comment

Choose a reason for hiding this comment

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

Sorry for delay, I think I missed the email with this request.

@amaltaro
Copy link
Contributor Author

Given that there is no replacement for this in the latest CouchDB version, I think it's better to get it merged sooner than later. At least we can have some experience with this code still in CouchDB 1.6.x. I need to have another look into this code though.

@amaltaro
Copy link
Contributor Author

And there is nothing else to be done here, merging.

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.

CouchDB no longer supports all_or_nothing option for bulk_docs API
4 participants