ARROW-9587: [FlightRPC][Java] clean up FlightStream/DoPut#8010
ARROW-9587: [FlightRPC][Java] clean up FlightStream/DoPut#8010lidavidm wants to merge 3 commits intoapache:masterfrom
Conversation
|
Some more fixes are needed here... |
|
@rymurr any thoughts here? This is my attempt at trying to stomp out various memory leak/usage issues in Flight, and a precursor to a way to measure per-RPC Arrow allocations, which is useful in deployments of Flight (e.g. to pinpoint problematic queries & identify if we've accidentally leaked allocations). |
rymurr
left a comment
rymurr
left a comment
There was a problem hiding this comment.
LGTM. Minor clarification required re the change to flight service. Really excited to see this patch! Let me know if I can help anywhere in the leak bug hunt.
There was a problem hiding this comment.
was this a permanent change or accidentally left in?
There was a problem hiding this comment.
Good catch, this was leftover from debugging.
There was a problem hiding this comment.
Maybe its just the wording of the comments but this seems like its theoretically possible for an observer to put a message between 187 and the lock gets aquired in 195. Is that true? The chance is prob pretty small and not easy to code for. Just wanted a bit of clarification
There was a problem hiding this comment.
Yes - it's possible because the observer is run in a separate thread (by gRPC) than the application, so the observer can trigger when the application is in the middle of close(). On the client side, draining the stream (as done here) prevents this case, but on the server side, we can't rely on this, unfortunately, hence the lock to protect things.
|
@lidavidm do you want another review or are you comfortable merging this? |
lidavidm
force-pushed
the
arrow-9587
branch
from
334ceac to
80abecb
Compare
|
@emkornfield I've rebased this and it should be good once tests pass. |
- Fix a bug where writes would hang forever for DoExchange - Make FlightRuntimeException#toString easier to read - Have DoPut reliably clean up the FlightStream when the call ends (instead of potentially closing it after gRPC thinks the call ends - this will be important for [ARROW-9586](https://issues.apache.org/jira/browse/ARROW-9586)) Closes apache#8010 from lidavidm/arrow-9587 Authored-by: David Li <[email protected]> Signed-off-by: David Li <[email protected]>
3 participants
Uh oh!
There was an error while loading. Please reload this page.