Management Command Database Locking #219

ajslater · 2022-02-05T02:21:06Z

Problem

If xapian-haystack management commands are used today with more than one worker it breaks with xapian.DatabaseLockError

Solution

This solution was cribbed from @karolyi in his issue report: #174 .
I implement a lockfile decorator around the update() and remove() methods.

Decisions

It paces the lockfile in the xapian-index directory, which should not affect performance.

Tests

This PR includes a new test_management_commands.py test suite that runs the management commands. This test suite fails if the lockfile isn't implemented as you can see in this branch: https://github.com/ajslater/xapian-haystack/actions/runs/1798091039

Workarounds

This PR can be used as inspiration for an easy workaround for this issue. Subclass XapianSearchBackend and BaseEngine with these changes. and use that in your Haystack settings. Example here: https://github.com/ajslater/codex/blob/develop/codex/search_engine.py

claudep

Thanks! We are on the right track :-) I'll work on removing Python 3.6 support to simplify things a bit.

.github/workflows/test.yml

requirements.txt

claudep · 2022-02-05T09:18:19Z

.github/workflows/test.yml

+        python-version: ["3.6", "3.9", "3.10"]
+        django-version: ["2.2", "3.1", "3.2"]
+        xapian-version: ["1.4.18"]
+        filelock-version: ["3.4.1"]


As already commented, I'd rather suggest dropping Python 3.6 support and we can get rid of filelock-version stuff in this file.

This is my first time using github actions, but this test suite appears to fail if filelock isn't explicitly installed in the "Install Django and other Python dependencies" step below. The matrix here seems a convenient place to store the version number next to the other version numbers. Also worth looking at is that you may have a comment on that i've added django 4.0 to the python 3.10 test.

I would simply drop any change to test.yml in this patch. Testing Python 3.10 or Django 4.0 should be another issue/PR. And filelock should be installed as part of dependencies installation.

Still todo?

I've restored the django 4.0 include version back to 3.2

The python 3.10 matrix & include requirements were added six months ago.

The filelock-version additions appear to be necessary. Specifically, the tests fail importing filelock if the dependency installations on line 75 does not install filelock.

Ah, I understand now, the test.yml doesn't use the requirements.txt. Sorry, you are right.

tests/xapian_tests/tests/test_management_commands.py

claudep · 2022-02-05T09:26:25Z

xapian_backend.py

+
+        # Ensure the lockfile exists
+        try:
+            self.lockfile.parent.mkdir(parents=True)


Shouldn't the parent path be created earlier in the backend __init__ method?

That's what i thought too. However the backend.clear() method destroys the entire xapian-index directory with shutil. xapian database or something ends up recreating it, maybe, but that happens after our file locking with update, so tests will fail without this and it seems that users who use the clear() or rebuild_index() method could jam themselves up on it too.

xapian_backend.py

claudep · 2022-02-05T10:05:09Z

I think we should still add a new custom connection option to opt out of the lock, so people not needing a multiprocess access have a way to keep better performances by not using the lock.

claudep · 2022-02-05T10:06:23Z

By the way, the minimal Python version is now 3.7.
77d1fc8

ajslater · 2022-02-06T20:06:51Z

I've fixed or commented on all the requested changes. Take another look. An addition I've made is that I noticed that backend.path can be an in-memory database so i've disabled the filelock in that case.

I can't find the comment, but I recall you asking if we might provide an option for people not to use filelocking in general. Should I add a boolean flag to connection_options?

tests/xapian_tests/tests/test_backend.py

xapian_backend.py

claudep · 2022-02-06T21:42:35Z

I can't find the comment, but I recall you asking if we might provide an option for people not to use filelocking in general. Should I add a boolean flag to connection_options?

Yes, please.

ajslater · 2022-02-07T00:04:30Z

Latest commits include the USE_LOCKFILE connection parameter with a short documentation line.

claudep · 2022-02-07T17:01:58Z

xapian_backend.py

@@ -175,13 +195,18 @@ def __init__(self, connection_alias, **connection_options):
                                       % connection_alias)

        self.path = connection_options.get('PATH')
+        self.use_lockfile = connection_options.get('USE_LOCKFILE', True)


Thanks for the flag addition. I would pop the option before the super().__init__ call to avoid passing it to the BaseSearchBackend.

Good idea. Done.
I've also casted it to a bool just so it doesn't end up with an unexpected value.

This was also changed to a setting from a connection option

README.rst

coveralls · 2022-02-07T20:11:38Z

Coverage decreased (-0.02%) to 97.558% when pulling 5633425 on ajslater:filelock-management-tests into 3fc4cfe on notanumber:master.

claudep · 2022-02-07T20:32:23Z

tests/xapian_tests/tests/test_management_commands.py

+            verbosity=2,
+            workers=10,
+            batchsize=2,
+        )


According to https://docs.djangoproject.com/en/4.0/ref/django-admin/#output-redirection, you should be able to capture stderr with the stderr= param of call_command.

I tried that and it didn't work.
It turns out for that to work the Command implementer has to explicitly send prints to print("blah", stderr=self.stderr), where self is BaseCommand. And I think these messages aren't even coming from python, but the underlying xapian database implementation.

And the underlying xapian database implementation is probably where all this file locking nonsense should be, but I digress.

Also, the failure mode for this test is only occasionally parsing that stderr string. Most of the time call_command() throws an exception and halts the test before the assert.

claudep · 2022-02-08T15:28:49Z

Sorry, I merged another pull request which forces you to rebase. Also, would you mind adding a line in the CHANGELOG?

ajslater · 2022-02-08T20:10:43Z

Merged from upstream and and added a CHANGELOG item.

claudep · 2022-02-09T19:46:05Z

Thanks, I think this is now ready. Only rebase and we are done.

commit 41b85e2 Author: AJ Slater <aj@slater.net> Date: Sun Feb 6 11:40:31 2022 -0800 use static method commit f8b895d Author: AJ Slater <aj@slater.net> Date: Sun Feb 6 11:38:08 2022 -0800 restore setup methods commit 984eb63 Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 18:24:31 2022 -0800 remove some duplication commit 829799e Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 16:13:13 2022 -0800 smaller batchsize fewer entries commit e61f0c5 Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 13:54:18 2022 -0800 more correct stderr check commit df114f4 Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 12:50:46 2022 -0800 move the directory creator back to the wrapper commit 0d23dac Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 12:40:12 2022 -0800 move directory creator to test setup commit 50258e6 Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 12:35:10 2022 -0800 move directory recreation to the test. remove self.lockfile commit 8760482 Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 12:27:08 2022 -0800 try to create the lockfile in backend.init commit 6d73f04 Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 12:24:21 2022 -0800 revise setup filelock version commit 839ed6a Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 12:23:04 2022 -0800 include doesn't inherit? commit 6b916fe Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 12:17:40 2022 -0800 forgot to install it commit 61d1788 Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 12:13:46 2022 -0800 add filelock version back commit 7debc3d Author: AJ Slater <aj@slater.net> Date: Sat Feb 5 12:02:07 2022 -0800 smaller number of blog entries

ajslater · 2022-02-09T22:09:31Z

Rebased on current master.

claudep · 2022-02-10T19:43:17Z

Thanks a lot for your work!

ajslater · 2022-02-10T21:18:40Z

Nice, looking forward to the release. Thanks for enforcing quality on this project!

ajslater changed the title ~~Filelock management tests~~ Management Command Database Locking Feb 5, 2022

claudep requested changes Feb 5, 2022

View reviewed changes

claudep reviewed Feb 6, 2022

View reviewed changes

tests/xapian_tests/tests/test_backend.py Outdated Show resolved Hide resolved

claudep reviewed Feb 6, 2022

View reviewed changes

xapian_backend.py Outdated Show resolved Hide resolved

ajslater mentioned this pull request Feb 7, 2022

tests for non-multiprocessing management commands #221

Merged

claudep reviewed Feb 7, 2022

View reviewed changes

README.rst Show resolved Hide resolved

claudep reviewed Feb 7, 2022

View reviewed changes

ajslater added 13 commits February 9, 2022 13:42

filelock for xapian management commands

f46b58d

fix data

7fbcc8a

don't inherit from other test

7b14b13

try harder to cause a conflict

f8b4e3d

reduce test time

d99eb55

don't need filelock to build xapian-bindings

0dac7fc

refined tests

283be5e

remove quote changes from lint and change dependancies to be more broad

0141043

remove unused import

214fdd7

mkdir exist_ok was added in python 3.5

4c6c211

USE_LOCKFILE connection_option

a8e7ee4

move use_lockfile before super(). change setting name

3773b3d

ajslater added 5 commits February 9, 2022 13:58

change to a setting

d44a413

typo

5dbedfe

changelog item

3791ef2

remove filelocked reference to self.path

9e91f5c

correct extra lines from rebase

bd82a49

ajslater force-pushed the filelock-management-tests branch from f164bcf to bd82a49 Compare February 9, 2022 22:03

ajslater added 2 commits February 9, 2022 14:05

restore extra line from rebase

f202ab2

restore blank line

5633425

claudep merged commit 870b48d into notanumber:master Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Management Command Database Locking #219

Management Command Database Locking #219

ajslater commented Feb 5, 2022 •

edited

claudep left a comment

claudep Feb 5, 2022

ajslater Feb 6, 2022

claudep Feb 6, 2022

claudep Feb 7, 2022

ajslater Feb 7, 2022

claudep Feb 7, 2022

claudep Feb 5, 2022

ajslater Feb 6, 2022

claudep commented Feb 5, 2022

claudep commented Feb 5, 2022

ajslater commented Feb 6, 2022 •

edited

claudep commented Feb 6, 2022

ajslater commented Feb 7, 2022

claudep Feb 7, 2022

ajslater Feb 7, 2022

ajslater Feb 7, 2022

coveralls commented Feb 7, 2022 •

edited

claudep Feb 7, 2022

ajslater Feb 7, 2022

ajslater Feb 7, 2022

ajslater Feb 7, 2022

claudep commented Feb 8, 2022

ajslater commented Feb 8, 2022

claudep commented Feb 9, 2022

ajslater commented Feb 9, 2022

claudep commented Feb 10, 2022

ajslater commented Feb 10, 2022

Management Command Database Locking #219

Management Command Database Locking #219

Conversation

ajslater commented Feb 5, 2022 • edited

Problem

Solution

Decisions

Tests

Workarounds

claudep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudep commented Feb 5, 2022

claudep commented Feb 5, 2022

ajslater commented Feb 6, 2022 • edited

claudep commented Feb 6, 2022

ajslater commented Feb 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudep commented Feb 8, 2022

ajslater commented Feb 8, 2022

claudep commented Feb 9, 2022

ajslater commented Feb 9, 2022

claudep commented Feb 10, 2022

ajslater commented Feb 10, 2022

ajslater commented Feb 5, 2022 •

edited

ajslater commented Feb 6, 2022 •

edited

coveralls commented Feb 7, 2022 •

edited