ARROW-9587: [FlightRPC][Java] clean up FlightStream/DoPut

[lidavidm]

• 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)

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9587

[lidavidm]

Some more fixes are needed here...

[lidavidm]

@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]

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.

[rymurr]

was this a permanent change or accidentally left in?

[lidavidm]

Good catch, this was leftover from debugging.

[rymurr]

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

[lidavidm]

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.

[emkornfield]

@lidavidm do you want another review or are you comfortable merging this?

[lidavidm]

@emkornfield I've rebased this and it should be good once tests pass.

[lidavidm]

• 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)

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9587

[lidavidm]

Some more fixes are needed here...

[lidavidm]

@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]

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.

[rymurr]

was this a permanent change or accidentally left in?

[lidavidm]

Good catch, this was leftover from debugging.

[rymurr]

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

[lidavidm]

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.

[emkornfield]

@lidavidm do you want another review or are you comfortable merging this?

[lidavidm]

@emkornfield I've rebased this and it should be good once tests pass.

[lidavidm]

• 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)

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9587

[lidavidm]

Some more fixes are needed here...

[lidavidm]

@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]

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.

[rymurr]

was this a permanent change or accidentally left in?

[lidavidm]

Good catch, this was leftover from debugging.

[rymurr]

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

[lidavidm]

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.

[emkornfield]

@lidavidm do you want another review or are you comfortable merging this?

[lidavidm]

@emkornfield I've rebased this and it should be good once tests pass.

[lidavidm]

• 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)

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9587

[lidavidm]

Some more fixes are needed here...

[lidavidm]

@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]

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.

[rymurr]

was this a permanent change or accidentally left in?

[lidavidm]

Good catch, this was leftover from debugging.

[rymurr]

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

[lidavidm]

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.

[emkornfield]

@lidavidm do you want another review or are you comfortable merging this?

[lidavidm]

@emkornfield I've rebased this and it should be good once tests pass.

[lidavidm]

• 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)

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9587

[lidavidm]

Some more fixes are needed here...

[lidavidm]

@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]

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.

[rymurr]

was this a permanent change or accidentally left in?

[lidavidm]

Good catch, this was leftover from debugging.

[rymurr]

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

[lidavidm]

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.

[emkornfield]

@lidavidm do you want another review or are you comfortable merging this?

[lidavidm]

@emkornfield I've rebased this and it should be good once tests pass.

[lidavidm]

• 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)

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9587

[lidavidm]

Some more fixes are needed here...

[lidavidm]

@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]

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.

[rymurr]

was this a permanent change or accidentally left in?

[lidavidm]

Good catch, this was leftover from debugging.

[rymurr]

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

[lidavidm]

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.

[emkornfield]

@lidavidm do you want another review or are you comfortable merging this?

[lidavidm]

@emkornfield I've rebased this and it should be good once tests pass.

[lidavidm]

• 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)

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9587

[lidavidm]

Some more fixes are needed here...

[lidavidm]

@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]

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.

[rymurr]

was this a permanent change or accidentally left in?

[lidavidm]

Good catch, this was leftover from debugging.

[rymurr]

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

[lidavidm]

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.

[emkornfield]

@lidavidm do you want another review or are you comfortable merging this?

[lidavidm]

@emkornfield I've rebased this and it should be good once tests pass.

[lidavidm]

• 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)

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9587

[lidavidm]

Some more fixes are needed here...

[lidavidm]

@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]

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.

[rymurr]

was this a permanent change or accidentally left in?

[lidavidm]

Good catch, this was leftover from debugging.

[rymurr]

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

[lidavidm]

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.

[emkornfield]

@lidavidm do you want another review or are you comfortable merging this?

[lidavidm]

@emkornfield I've rebased this and it should be good once tests pass.