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

Send large arrow binaries as chunks, add client-level Websocket heartbeat #1209

Merged
merged 7 commits into from
Oct 7, 2020

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Sep 28, 2020

This PR implements chunked sending/chunked loading of binaries in Perspective's websocket implementations in both Python and Javascript. Apache Arrow binaries above a certain size will be sent to the Perspective client in chunks, which prevents the Websocket from locking on a particularly large binary and closing due to a timeout, as once that binary is on the wire all heartbeat messages/other messages are blocked from being sent/received.

The default chunk threshold is configured to be 200MB, and arrows will be transported in 50MB chunks. In Python, these settings can be altered in the PerspectiveManager constructor by initializing a new PerspectiveManager with chunk_threshold and chunk_size, which are defined in bytes:

manager = PerspectiveManager(
    chunk_threshold=500 * 1000 * 1000,  # in bytes
    chunk_size=100 * 1000 * 1000
)

In NodeJS, these settings can be altered on the WebsocketManager:

const manager = new WebsocketManager();
manager.chunk_threshold = 500 * 1000 * 1000;  // in bytes
manager.chunk_size = 100 * 1000 * 1000;

Additionally, this PR implements a client-level heartbeat message (ping/pong) between Perspective client and server, which should aid in debugging as heartbeat messages now show up in the browser debug console. In Tornado, servers previously set up with websocket_ping_interval should remove their websocket_ping_interval setting and depend on the client-level heartbeats to keep the Perspective websocket connection alive.

Changelog

  • Implements chunked binary sending on PerspectiveTornadoServer in Python and WebSocketManager in JS
  • Implements chunked binary receiving in WebSocketClient in JS and PerspectiveTornadoClient in Python
  • Adds tests for chunked send/receive in Python
  • Adds ping/pong heartbeat for Websockets

msg.is_transferable = true;
msg.arrow_length = arrow.byteLength;
Copy link
Member

Choose a reason for hiding this comment

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

I nominate we combine this property with is_transferable, since these are now always coincident, and let the presence of arrow_length property imply is_transferable.

Also - maybe "buffer_length" is more conventional term here for arrow_length? We don't really opine on the nature of a msg at this level of the API anywhere else.

req.ws.send(transferable[0]);
setTimeout(() => {
this._post_chunked(req, arrow, 0, this.chunk_size, arrow.byteLength);
}, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a need to defer this first step? We have the ArrayBuffer in hand already, and it seems deferring here may simply introduce another event-loop-flush delay to arrow-containing messages that are only a single chunk in length.

Defaults to 200MB.
chunk_size (:obj:`int`): Binary messages above ``chunk_threshold``
will be passed in chunks of this length (in bytes). Defaults
to 20MB.
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, I nominate we combine chunk_threshold and chunk_size into a single variable. Under these default values even, it is possible/likely to get scenarios with average chunk size way above the chunk_size param by targetting e.g. 190mb requests, not large enough to trigger the threshold but 9.5x over the intended loop chunk size.

Ideally, this is a variable the developer does not ever need to touch, and may even be removed at some point if we can automate it. Something like this:

chunk_size (:obj:`int`): Binary messages will not exceed this length
    (in bytes);  payloads above this limit will be chunked
    across multiple messages. Defaults to `20971520` (20MB), and
    be disabled entirely with `None`.

@sc1f sc1f force-pushed the tornado-heartbeat branch from 9f58bef to 3ac2d63 Compare October 5, 2020 19:35
@texodus texodus force-pushed the tornado-heartbeat branch from c05d686 to ad481b9 Compare October 6, 2020 02:53
@texodus
Copy link
Member

texodus commented Oct 7, 2020

Combined chunk_size and chunk_threshold kwargs, as per review. I've additionally added the private _chunk_sleep for debugging, though this has been set to 0 by default see here

Fix & test also for an issue with asynchronous logic, namely that once a client is in "pending_binary" state, it cannot process other messages. This is only sometimes an issue, as clients tend to serialize themselves by await-ing responses to queries; when it occurs though, the client will hang. Note this change is per-websocket, and does not affect "ping" messages, since they are processed before the post() locking and response dispatch logic is run.

Before: a call to num_rows() after an un-awaited call to to_arrow() interleaves the response, between messages 1 and 2 of a 3 message binary stream.

Screen Shot 2020-10-05 at 11 09 38 PM

After: num_rows() is deferred until the stream is finished, in an elaborated _chunk_sleep. The response to num_rows() is deferred until the stream is complete.

Screen Shot 2020-10-07 at 2 16 00 AM

@texodus texodus merged commit 26321f0 into master Oct 7, 2020
@texodus texodus deleted the tornado-heartbeat branch October 7, 2020 07:20
@texodus texodus added bug Concrete, reproducible bugs Python enhancement Feature requests or improvements JS and removed bug Concrete, reproducible bugs labels Oct 7, 2020
@texodus texodus mentioned this pull request Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests or improvements JS Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants