Skip to content

Conversation

@wesm
Copy link
Member

@wesm wesm commented Oct 4, 2017

This was one part of ARROW-1134 that we can push through. I had to do some refactoring since there was a mutex in one of the base file interfaces. It doesn't appear that this will impact parquet-cpp or other Arrow users

@wesm
Copy link
Member Author

wesm commented Oct 4, 2017

Hm, seems I have some more work to do here. I'm not sure about the best strategy for threadsafety by default in the RandomAccessFile::ReadAt functions. Before there was a default virtual implementation, but the mutex was visible in the header. We could add a PIMPL to RandomAccessFile but that might be overkill.

The default option is that I add basic implementations to the classes that are breaking the build currently

wesm added 2 commits October 6, 2017 14:31
Change-Id: I0bb5695fa1cf7a591f7333bb8c63487212d0d749
…ix deadlocks in ReadableFile

Change-Id: I9d09b0daaa6a00562d5ed9e63e704258a3687b58
@wesm
Copy link
Member Author

wesm commented Oct 6, 2017

This should be ready to go once the tests pass

@wesm
Copy link
Member Author

wesm commented Oct 6, 2017

hm, there's a test failure, it looks like I borked something with threadsafety. taking another look

Change-Id: I738a713e0a17cb6c8cdf3c2eaa3191fb39253f06
@wesm
Copy link
Member Author

wesm commented Oct 6, 2017

@kou it's possible that this patch introduces threadsafety issues in the GIO wrapper implementations if you use the RandomAccessFile::ReadAt functions at all, just as a heads up

Change-Id: Ie5e1be5e7d4c119fde229c55d88c7aaaf40388cd
@kou
Copy link
Member

kou commented Oct 6, 2017

Thanks for the heads-up.
I'll add thread-safe ReadAt implementation to garrow::GIOInputStream later.

@wesm
Copy link
Member Author

wesm commented Oct 6, 2017

Waiting for https://ci.appveyor.com/project/wesm/arrow/build/1.0.1269 since I twiddled some symbol visibility stuff

@wesm
Copy link
Member Author

wesm commented Oct 7, 2017

+1

@asfgit asfgit closed this in b29b065 Oct 7, 2017
@wesm wesm deleted the ARROW-1641 branch October 7, 2017 19:48
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.

2 participants