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

Python Client import_table Should not also Close the Writer #4777

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

nbauernfeind
Copy link
Member

@nbauernfeind nbauernfeind commented Nov 6, 2023

This fixes last night's nightly-ci failure. Note that the failure is difficult to reproduce as there is some racing involved. However, do note that DoPut requires the stream to be cancelled before it is considered complete. Since there are python tests that round-trip to ensure the data uploaded is readable we can be confident that the DoPut is being closed by the pyarrow library. (I did attempt to follow the code path through the cpp implementation, but it was hard for me to find the specific WriteTable call that is being invoked in this context.)

====================================================================== |  
  | ERROR [30.064s]: test_import_table_strings (test_session.SessionTestCase) |  
  | ---------------------------------------------------------------------- |  
  | Traceback (most recent call last): |  
  | File "/project/pydeephaven/_arrow_flight_service.py", line 34, in import_table |  
  | writer.close() |  
  | File "pyarrow/_flight.pyx", line 1166, in pyarrow._flight.MetadataRecordBatchWriter.close |  
  | File "pyarrow/_flight.pyx", line 62, in pyarrow._flight.check_flight_status |  
  | pyarrow._flight.FlightCancelledError: Flight cancelled call, with message: Received RST_STREAM with error code 8. gRPC client debug context: UNKNOWN:Error received from peer ipv4:172.21.0.2:10000 {created_time:"2023-11-06T06:25:08.49824529+00:00", grpc_status:1, grpc_message:"Received RST_STREAM with error code 8"}. Client context: OK |  
  |   |  
  | The above exception was the direct cause of the following exception: |  
  |   |  
  | Traceback (most recent call last): |  
  | File "/project/tests/test_session.py", line 177, in test_import_table_strings |  
  | new_table = self.session.import_table(pa_table) |  
  | File "/project/pydeephaven/session.py", line 486, in import_table |  
  | return self.flight_service.import_table(data=data) |  
  | File "/project/pydeephaven/_arrow_flight_service.py", line 39, in import_table |  
  | raise DHError("failed to create a Deephaven table from Arrow data.") from e |  
  | pydeephaven.dherror.DHError: failed to create a Deephaven table from Arrow data. |  
 ```

Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

LGTM

@nbauernfeind nbauernfeind enabled auto-merge (squash) November 6, 2023 18:28
@nbauernfeind nbauernfeind merged commit bb33ccd into deephaven:main Nov 6, 2023
17 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants