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

ARROW-5533: [C++] [Plasma] make plasma client thread safe #4503

Closed
wants to merge 8 commits into from

Conversation

zhijunfu
Copy link
Contributor

@zhijunfu zhijunfu commented Jun 9, 2019

Make plasma client thread safe, so it's guaranteed when the PlasmaBuffer for an object destructs, the call to plasma_client.Release(object_id) is properly protected. Refer to here for background.

@robertnishihara @pcmoritz

@robertnishihara
Copy link
Contributor

This looks good to me, but it seems like there might be some related test failures in Travis.

@zhijunfu zhijunfu force-pushed the plasma-client-mutex branch from a5b9477 to 6249e98 Compare June 13, 2019 10:03
@codecov-io
Copy link

codecov-io commented Jun 13, 2019

Codecov Report

Merging #4503 into master will increase coverage by 0.43%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4503      +/-   ##
=========================================
+ Coverage   88.56%     89%   +0.43%     
=========================================
  Files         860     707     -153     
  Lines      108022   95256   -12766     
  Branches     1253       0    -1253     
=========================================
- Hits        95674   84782   -10892     
+ Misses      12069   10474    -1595     
+ Partials      279       0     -279
Impacted Files Coverage Δ
cpp/src/plasma/client.cc 97.1% <100%> (+0.14%) ⬆️
cpp/src/plasma/thirdparty/ae/ae.c 70.75% <0%> (-0.95%) ⬇️
go/arrow/ipc/writer.go
go/arrow/math/uint64_amd64.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/ipc/file_reader.go
js/src/enum.ts
go/arrow/array/builder.go
js/src/Arrow.node.ts
js/src/schema.ts
... and 146 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fc91cd...c82ad93. Read the comment docs.

@pitrou pitrou changed the title ARROW-5533: make plasma client thread safe ARROW-5533: [C++] [Plasma] make plasma client thread safe Jun 13, 2019
@zhijunfu
Copy link
Contributor Author

@robertnishihara all travis tests pass now. I changed mutex to recursive_mutex as some plasma client interfaces are called from other ones, thus the lock needs to be reentrant.
I'll also file a PR on the ray side to verify the change.

@pcmoritz
Copy link
Contributor

LGTM, but let's wait for the Ray test suite to pass before merging it!

@pcmoritz pcmoritz force-pushed the plasma-client-mutex branch from 6249e98 to c82ad93 Compare June 17, 2019 05:57
@pcmoritz
Copy link
Contributor

rebased to include #4494

cpp/src/plasma/client.cc Outdated Show resolved Hide resolved
Copy link

@shengjun1985 shengjun1985 left a comment

Choose a reason for hiding this comment

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

Thank you for fix, but I have no idea why CI failed.

@zhijunfu
Copy link
Contributor Author

@shengjun1985 that one is specific to mac and seems unrelated.

@wesm
Copy link
Member

wesm commented Jun 19, 2019

Merging. CI failure looks unrelated to me

@wesm wesm closed this in 726f90f Jun 19, 2019
kou pushed a commit that referenced this pull request Dec 18, 2020
There's a pr #4503 make PlasmaClient thread safe now, but in the doc there's "Note that a PlasmaClient object is not thread safe.", which causes misunderstanding.

Closes #8958 from offthewall123/update_plasma_doc_PlasmaClient_ThreadSafe

Authored-by: xudingyu <dingyu.xu@intel.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
There's a pr apache#4503 make PlasmaClient thread safe now, but in the doc there's "Note that a PlasmaClient object is not thread safe.", which causes misunderstanding.

Closes apache#8958 from offthewall123/update_plasma_doc_PlasmaClient_ThreadSafe

Authored-by: xudingyu <dingyu.xu@intel.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants