Skip to content
This repository was archived by the owner on Feb 10, 2021. It is now read-only.

Add file based locks #99

Merged
merged 2 commits into from
Nov 10, 2016
Merged

Add file based locks #99

merged 2 commits into from
Nov 10, 2016

Conversation

mrocklin
Copy link
Member

I've also reverted #98 . It looks like libhdfs3 already does retries.

@mrocklin
Copy link
Member Author

cc @martindurant

@martindurant
Copy link
Member

So, lock around everything...
If you are unconcerned with the the extra overhead, this is fine with me, although I'm not 100% convinced that this will solve unpickling errors in every case.

We should have a requirements.txt now.

@mrocklin
Copy link
Member Author

Are there other situations that concern you?

I believe that all access to libhdfs3 is now controlled under a lock.

On Thu, Nov 10, 2016 at 9:13 AM, Martin Durant [email protected]
wrote:

So, lock around everything...
If you are unconcerned with the the extra overhead, this is fine with me,
although I'm not 100% convinced that this will solve unpickling errors in
every case.

We should have a requirements.txt now.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#99 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AASszFVenm94mD_Sl1ZSvxN9NxXmAxcXks5q8yaVgaJpZM4KuqAo
.

@martindurant
Copy link
Member

The methods under HDFile have not been done. Whilst acting on a file may be immune to this because it's a separate data-node connection (I don't know), at least open must be protected. Probably best to protect everything.

I don't have concrete situations in mind, just a vague feeling that we might have to revisit this.

@mrocklin
Copy link
Member Author

OK, I've extended locks to almost all methods. We don't lock around close or flush. These will cause deadlocks, I suspect because they are called within __del__. I'm going to let these lie for now.

I've benchmarked this against toy data and verified that the way the locks are used now they do not significantly impact write or read performance.

@mrocklin
Copy link
Member Author

@martindurant this could use your review and, if you think it's ok, merge.

@martindurant martindurant merged commit b493dbf into dask:master Nov 10, 2016
@mrocklin mrocklin deleted the lock-it-all branch November 14, 2016 23:25
@pitrou
Copy link
Member

pitrou commented Nov 21, 2016

After diagnosing some additional woes on Travis-CI, it seems this PR introduced some deadlocks (which I can reproduce at home on the distributed test suite). See dask/distributed#687 (comment)

@pitrou pitrou mentioned this pull request Nov 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants