Skip to content

Conversation

@Suor
Copy link
Contributor

@Suor Suor commented Sep 3, 2019

This locks employs hardlink trick, which works on NFS and alike unlike
fcntl/open locks.

Closes #1918.

Suor added 2 commits September 6, 2019 16:29
This locks employs hardlink trick, which works on NFS and alike unlike
fcntl/open locks.

Closes treeverse#1918.
@Suor Suor force-pushed the nfs branch 2 times, most recently from de8d4e3 to 9111bf5 Compare September 8, 2019 06:41
@Suor
Copy link
Contributor Author

Suor commented Sep 8, 2019

So things still might not work under Python 2 or very old sqlite3 version. Which is an issue than will solve itself ).

@efiop
Copy link
Contributor

efiop commented Sep 8, 2019

@Suor FYI: tests on py2.7 on windows are failing.

dvc/updater.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't set lifetime here, is there some default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, 5 seconds. Plus 10 seconds, which flufl uses internally to defend against time desync.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is not acceptable, I can see it taking that long for the updater on bad network. Just set the longest time by default in Lock and don't configure it unnecessarily each time you use Lock. It is the way our old locks worked, it should stay that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Suor ?

@Suor
Copy link
Contributor Author

Suor commented Sep 10, 2019

Reverted to zc.lockfile for Python 2, it's very hard to make flufl locks work on Python2.7/Windows and we gave up with sqlite3 URIs for Python 2 anyway.

Return to zc.lockfile lock in Python 2, NFS is not supported there
anyway since sqlite3 doesn't support URIs.
tmp_dir (str): a directory to store claim files.
"""

def __init__(self, lockfile, lifetime=None, tmp_dir=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for setting a long default lifetime. Don't see this one used anywhere now

Suggested change
def __init__(self, lockfile, lifetime=None, tmp_dir=None):
def __init__(self, lockfile, tmp_dir=None):

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record: will remove this myself right after merge to move along faster.

@efiop efiop merged commit 05adcf9 into treeverse:master Sep 11, 2019
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.

dvc: .dvc/lock and sqlite lock on NFS and CIFS

2 participants