ARROW-5063: [FlightRPC][Java] Test that Flight client connections are independent #4648

ghost · 2019-06-21T12:53:26Z

This adds a test that creates two Flight clients to the same server, then closes one Flight client while the other still has an operation in progress, in order to ensure that gRPC doesn't share connections between the clients.

BryanCutler

Thanks for checking this out @lihalite ! The issue I was seeing was probably due to some grpc internals and I'm not sure of a better test to reproduce. Regardless, the fix here looks good and I think it makes things more consistent. I just had 2 minor comments.

BryanCutler · 2019-06-21T16:45:38Z

java/flight/src/main/java/org/apache/arrow/flight/FlightClient.java

could you rename incomingAllocator -> allocator to be consistent with the the other classes?

BryanCutler · 2019-06-21T16:47:12Z

java/flight/src/test/java/org/apache/arrow/flight/TestSharedConnection.java

How about renaming this file to something a little more general, like "TestFlightClient.java"?

ghost · 2019-06-21T17:30:42Z

Thanks for taking a look, I made those two changes.

You may also tangentially appreciate #4652 for improved error handling in Flight/Java.

codecov-io · 2019-06-21T19:07:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@be16de0). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4648   +/-   ##
=========================================
  Coverage          ?   89.41%           
=========================================
  Files             ?      654           
  Lines             ?    93352           
  Branches          ?        0           
=========================================
  Hits              ?    83468           
  Misses            ?     9884           
  Partials          ?        0

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 be16de0...0f62556. Read the comment docs.

jacques-n

Ignore... Wrong pr

BryanCutler

LGTM

BryanCutler · 2019-06-21T19:45:08Z

While you're here @jacques-n , any issue with this change?

ghost · 2019-06-21T19:47:03Z

Jacques commented on the JIRA https://issues.apache.org/jira/browse/ARROW-5063
Maybe we can back out the change to the allocator, but keep the test?

BryanCutler · 2019-06-21T20:50:27Z

Comment from ARROW-5063:

I think we're trying to solve this wrong way. The idea that a client has its own allocator that is closed as it closes out to make sure it doesn't leak any memory is a good thing.

That is fine @jacques-n , the problem I had was when I called FlightClient.close() it also calls channel.shutdown() which seemed to break the grpc channel for other clients that were connected and caused a crash. I guess I'll have to try out my code again to see if it is still an issue, but I basically followed IntegrationTestClient that loops over locations and make a client for each

for (FlightEndpoint endpoint : info.getEndpoints()) {   
      List<Location> locations = endpoint.getLocations();
      for (Location location : locations) {
        FlightClient readClient = FlightClient.builder(allocator, location).build();
        ...
        // readClient.close()

Because of the error, I couldn't close the client and to check for leaks. That's why I thought it made more sense to not couple closing the allocator with closing the channel.

jacques-n · 2019-06-24T12:51:24Z

There should be no state sharing of independent channels from distinct clients. If so, it sounds like we have another bug and this is working around it instead of fixing it. To confirm, the client caused another channel closure or closing the client triggered the server to close all channels?

…

On Fri, Jun 21, 2019, 1:50 PM Bryan Cutler ***@***.***> wrote: Comment from ARROW-5063: I think we're trying to solve this wrong way. The idea that a client has its own allocator that is closed as it closes out to make sure it doesn't leak any memory is a good thing. That is fine @jacques-n <https://github.com/jacques-n> , the problem I had was when I called FlightClient.close() it also calls channel.shutdown() which seemed to break the grpc channel for other clients that were connected and caused a crash. I guess I'll have to try out my code again to see if it is still an issue, but I basically followed IntegrationTestClient that loops over locations and make a client for each for (FlightEndpoint endpoint : info.getEndpoints()) { List<Location> locations = endpoint.getLocations(); for (Location location : locations) { FlightClient readClient = FlightClient.builder(allocator, location).build(); ... // readClient.close() Because of the error, I couldn't close the client and to check for leaks. That's why I thought it made more sense to not couple closing the allocator with closing the channel. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4648?email_source=notifications&email_token=AABMYNVG56BW6G4DCSPB7NTP3U5JLA5CNFSM4H2RNMOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYJRWKQ#issuecomment-504568618>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABMYNWLSDGU4QORJ7PN4X3P3U5JLANCNFSM4H2RNMOA> .

ghost · 2019-06-24T17:00:47Z

It sounds like Bryan found that closing one client channel would close other client channels. This adds a simple unit test for that scenario, but we should do more investigation (probably the unit test can be made more robust to ensure that multiple clients are active at once).

AFAIK, some gRPC implementations do share channels internally; I believe C++ is one of them, but it also doesn't require you to close a channel explicitly.

ghost · 2019-06-24T17:07:03Z

Changed:

Revert changes to Flight client
More thoroughly test that closing one client doesn't affect an ongoing operation on another client

BryanCutler · 2019-06-24T18:10:04Z

To confirm, the client caused another channel closure or closing the client triggered the server to close all channels?

From what I remember, multiple clients had the same location and seemed to share a channel. So closing one client, then trying to read from the next failed. I tried to debug what was going on with grpc internally but it was a bit confusing.. I'll try to reproduce the issue again and I'll open another JIRA if it still happens.

BryanCutler

The test looks fine and I think we can merge it. @lihalite can you change the PR title/description to match the changes?

ghost · 2019-06-24T18:15:19Z

@BryanCutler sure, updated.

wesm · 2019-06-24T22:58:50Z

Looks like this is failing due to checkstyle

ghost · 2019-06-25T17:57:07Z

This is now failing due to JPype, I can rebase once that's fixed.

wesm · 2019-06-25T19:31:05Z

I just posted a (temporary) fix for the jpype issue, will merge that once I have a passing build

ghost · 2019-06-26T13:46:42Z

Rebased, personal Travis: https://travis-ci.com/lihalite/arrow/builds/116983004

wesm

+1

wesm · 2019-06-27T00:50:14Z

@lihalite I think this PR has a conflict with the application metadata patch

BryanCutler · 2019-06-27T02:26:00Z

It looks like a quick fix, I'll post a PR

emkornfield · 2019-06-27T02:30:45Z

I think I fixed this already. PR pending review

…

On Wednesday, June 26, 2019, Bryan Cutler ***@***.***> wrote: It looks like a quick fix, I'll post a PR — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4648?email_source=notifications&email_token=AEIKYDQKWKUQ7AA7HPSKFRTP4QQMJA5CNFSM4H2RNMOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYVPCTQ#issuecomment-506130766>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEIKYDUK24CDEQBDHLDJAD3P4QQMJANCNFSM4H2RNMOA> .

BryanCutler · 2019-06-27T02:54:17Z

Oops, I didn't see that. I'll close mine.

… independent This adds a test that creates two Flight clients to the same server, then closes one Flight client while the other still has an operation in progress, in order to ensure that gRPC doesn't share connections between the clients. Author: David Li <[email protected]> Closes apache#4648 from lihalite/arrow-5063 and squashes the following commits: 7002bc3 <David Li> Add test for independent Flight client shutdown in Java

BryanCutler reviewed Jun 21, 2019

View reviewed changes

jacques-n reviewed Jun 21, 2019

View reviewed changes

BryanCutler approved these changes Jun 21, 2019

View reviewed changes

BryanCutler reviewed Jun 24, 2019

View reviewed changes

ghost changed the title ~~ARROW-5063: [FlightRPC][Java] Don't create child allocator in FlightClient~~ ARROW-5063: [FlightRPC][Java] Test that Flight client connections are independent Jun 24, 2019

Add test for independent Flight client shutdown in Java

7002bc3

wesm approved these changes Jun 26, 2019

View reviewed changes

wesm closed this in 0466025 Jun 26, 2019

asfimport mentioned this pull request Jun 27, 2019

[Java] FlightClient should not create a child allocator #21555

Closed

ARROW-5063: [FlightRPC][Java] Test that Flight client connections are independent #4648

ARROW-5063: [FlightRPC][Java] Test that Flight client connections are independent #4648

Uh oh!

Conversation

ghost commented Jun 21, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

BryanCutler Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

BryanCutler Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 21, 2019

Uh oh!

codecov-io commented Jun 21, 2019

Codecov Report

Uh oh!

jacques-n left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Jun 21, 2019

Uh oh!

ghost commented Jun 21, 2019

Uh oh!

BryanCutler commented Jun 21, 2019

Uh oh!

jacques-n commented Jun 24, 2019 via email

Uh oh!

ghost commented Jun 24, 2019

Uh oh!

ghost commented Jun 24, 2019

Uh oh!

BryanCutler commented Jun 24, 2019

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 24, 2019

Uh oh!

wesm commented Jun 24, 2019

Uh oh!

ghost commented Jun 25, 2019

Uh oh!

wesm commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 26, 2019

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

wesm commented Jun 27, 2019

Uh oh!

BryanCutler commented Jun 27, 2019

Uh oh!

emkornfield commented Jun 27, 2019 via email

Uh oh!

BryanCutler commented Jun 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ghost commented Jun 21, 2019 •

edited by ghost

Loading

jacques-n left a comment •

edited

Loading

wesm commented Jun 25, 2019 •

edited

Loading