ARROW-17407: [Doc][FlightRPC] Flight/gRPC best practices #13873

rok · 2022-08-14T02:44:53Z

We want to provide best practices and debugging section in:

cpp flight docs
python flight docs
java flight docs
R flight docs

github-actions · 2022-08-14T02:45:12Z

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

zeroshade · 2022-08-14T16:38:34Z

@rok Can we also add best practices and debugging sections in the Go docs too?

This can be done by either a README.md in the go/arrow/flight directory or by having a "package comment" in the directory (a comment directly above the package flight line in one of the .go files (convention is usually a file named doc.go).

Would you prefer for me to add it to this branch?

rok · 2022-08-15T14:33:59Z

@rok Can we also add best practices and debugging sections in the Go docs too?

Thanks for chiming in @zeroshade! I was going to ask you where to put the Go specific content :).
I'll add it and ping you back.

zeroshade · 2022-08-15T16:02:18Z

@rok Sounds great! Have a look at go/arrow/doc.go or go/parquet/doc.go for some examples.

https://tip.golang.org/doc/comment also gives a good rundown of how the format of the comment gets translated into the formatting of the rendered docs on pkg.go.dev (TL;DR it's similar to just using markdown)

rok · 2022-08-15T23:37:33Z

@zeroshade I've added the Go doc. Please check if it makes sense, it's my first time touching Go docs.

rok · 2022-08-16T00:02:13Z

@lidavidm I'm now linking to c++ from python/java/R docs which is not ideal, but duplicating text is not either. Any thoughts?

drin · 2022-08-16T00:16:38Z

I'm now linking to c++ from python/java/R docs which is not ideal, but duplicating text is not either. Any thoughts?

IMO, I would think there should be a common area for the text since that is common across languages. Then, the c++ docs can include code snippets as appropriate which don't seem too relevant to the other languages.

The "Arrow Flight RPC" section under "Specifications and Protocols" seems like a good place to me, since those descriptions seem to be more related to best practices of using the protocol overall.

drin · 2022-08-16T00:18:23Z

The code snippet could potentially be moved to a cookbook type thing too?

lidavidm · 2022-08-16T00:18:25Z

What's wrong with linking to the C++ docs?

The format/ page for Flight RPC is meant more for things all implementations agree on, I don't think these tips really go there

lidavidm · 2022-08-16T00:18:58Z

Also, linking to the C++ docs isn't applicable in all situations, they don't apply to Java for instance.

lidavidm · 2022-08-16T00:20:32Z

The same goes for Go.

If we want to centralize things, we should think about where to do that. And then all the language docs can link there. (I think Sphinx has a 'tabs' extension we could use for the language-specific snippets too.)

drin · 2022-08-16T00:22:06Z

What's wrong with linking to the C++ docs?

I don't think there's anything wrong with it. I was just thinking through alternatives since Rok didn't seem to like it.

Also, linking to the C++ docs isn't applicable in all situations, they don't apply to Java for instance.

Maybe I misread that portion of the PR, but I think it references it at the moment? I don't know the details, so not sure if some of the descriptions need disentangling

lidavidm · 2022-08-16T00:23:24Z

The Java docs link to the C++ docs - but the C++ docs don't apply to Java. The bit about allocations doesn't apply to Java at all, for instance.

drin · 2022-08-16T00:28:04Z

options that make the most sense to me are:

make a new page, perhaps under the "development" section?
leave the structure as is, and let a PR for ARROW-17378 and/or ARROW-6390 re-structure?

lidavidm · 2022-08-16T00:29:48Z

Maybe we should consider a separate tutorial section then? Even if they're language-specific? (Well, 'tutorials and guides' or something…) The Flight tutorials could probably be made to work with the same 'tab' structure so they can cover multiple languages at once (though that should be used sparingly)

drin · 2022-08-16T00:37:29Z

Assuming this tab structure:
I like the idea of a tab structure in cases where code snippets are small (or whatever text will go in the tabs), which seems to apply to what I see in this documentation. I also think that for something like flight, the focus is more on the protocol and communication so it's a case where there is a lot of conceptual overlap so tabs may work well here.

Otherwise, I don't have strong opinions about "best practices" being in, or separate from, a tutorial section.

lidavidm · 2022-08-16T22:29:42Z

I didn't get to this today, should hopefully get to look at it this week

wjones127

Thank you for starting this @rok! I've added a few suggestions.

docs/source/cpp/flight.rst

lidavidm · 2022-08-17T18:17:52Z

docs/source/cpp/flight.rst

+When using default gRPC transport options can be passed to it via
+:member:`arrow::flight::FlightClientOptions::generic_options`. For example:


This is less a best practice, more of a "howto" so I think this should go in the cookbook

docs/source/cpp/flight.rst

lidavidm · 2022-08-17T18:23:59Z

docs/source/cpp/flight.rst

+This is what you are seeing now.
+bounded thread pool -> Reject connections / requests when under load, and have
+clients retry with backoff. This also gives an opportunity to retry with a
+different node. Not everyone gets serviced but quality of service stays consistent.


we should add code to configure these modes

You mean like setting GRPC_ARG_MIN_RECONNECT_BACKOFF_MS to 200ms for bounded and 0ms for unbounded? Perhaps another setting I'm not aware of?

Added GRPC_ARG_MIN_RECONNECT_BACKOFF_MS example.

lidavidm · 2022-08-17T18:24:22Z

docs/source/cpp/flight.rst

+
+1. A stale connection can be closed using
+   :member:`arrow::flight::FlightClientOptions::stop_token`. This requires recording the
+   stop token at connection establishment time.


this needs code samples

(it would be great if all code samples linked to the cookbook)

Perhaps we could add samples for now and they would be moved out as cookbooks become available?
Also I don't see an issue if we duplicate atomic pieces of the cookbooks here.

I would much rather they be cookbook samples so that they actually get run in CI. It's probably good to still have a short snippet here, but I worry about those kinds of things bit-rotting without them actually getting exercised like the Cookbook does.

Yeah, agreed.

We can maybe merge this then add issues for moving each of these examples to the cookbook.

docs/source/cpp/flight.rst

go/arrow/flight/doc.go

zeroshade

Looks pretty good to me on the Go side though some of the comments aren't accurate or relevant to the Go library, i've made some suggestions.

go/arrow/flight/doc.go

zeroshade · 2022-08-18T14:55:15Z

go/arrow/flight/doc.go

+// https://issues.apache.org/jira/browse/ARROW-15764 proposes a caching
+// optimisation for server side, but it was not yet implemented.


probably should mention that the caching optimization is specifically for C++ in that issue.

zeroshade · 2022-08-18T14:57:04Z

go/arrow/flight/doc.go

+// gRPC will spawn an unbounded number of threads for concurrent clients. Those
+// threads are not necessarily cleaned up (cached thread pool in java parlance).
+// glibc malloc clears some per thread state and the default tuning never clears
+// caches in some workloads. But you can explicitly tell malloc to dump caches.
+// See https://issues.apache.org/jira/browse/ARROW-16697 as an example.


This would actually be incorrect for the Go implementation since it uses goroutines which do not have a 1-to-1 relationship with OS threads. It wouldn't fit the "thread pool" description. We can probably remove this entire paragraph for the Go docs here.

go/arrow/flight/doc.go

zeroshade · 2022-08-18T14:59:26Z

go/arrow/flight/doc.go

+// Closing unresponsive connections
+//
+// * A stale connection can be closed using CallOptions.stop_token. This requires
+// recording the stop token at connection establishment time.
+// * Use client timeout
+// * here is a long standing ticket for a per-write/per-read timeout instead of a per
+// call timeout (https://issues.apache.org/jira/browse/ARROW-6062), but this is not
+// (easily) possible to implement with the blocking gRPC API. For now one can also do
+// something like set up a background thread that calls cancel() on a timer and have
+// the main thread reset the timer every time a write operation completes successfully
+// (that means one needs to use to_batches() + write_batch and not write_table).


We should add some comment here about the ability to use the context.Context that is passed to requests to implement a timeout via https://pkg.go.dev/context#WithTimeout or the ability to cancel via https://pkg.go.dev/context#WithCancel

Added something, but I'm not sure I understand enough for it to make sense :).

rok

Thanks for the review everyone! I've gone through your comments and hopefully used them productively.

docs/source/cpp/flight.rst

rok · 2022-08-18T19:37:30Z

docs/source/cpp/flight.rst

+This is what you are seeing now.
+bounded thread pool -> Reject connections / requests when under load, and have
+clients retry with backoff. This also gives an opportunity to retry with a
+different node. Not everyone gets serviced but quality of service stays consistent.


You mean like setting GRPC_ARG_MIN_RECONNECT_BACKOFF_MS to 200ms for bounded and 0ms for unbounded? Perhaps another setting I'm not aware of?

rok · 2022-08-18T19:38:49Z

docs/source/cpp/flight.rst

+
+1. A stale connection can be closed using
+   :member:`arrow::flight::FlightClientOptions::stop_token`. This requires recording the
+   stop token at connection establishment time.


Perhaps we could add samples for now and they would be moved out as cookbooks become available?
Also I don't see an issue if we duplicate atomic pieces of the cookbooks here.

rok · 2022-09-02T18:40:32Z

I've addressed the feedback (hopefully all) and opened a cookbook issue (apache/arrow-cookbook#251).
Would appreciate another look! :)

github-actions · 2022-09-02T18:42:34Z

Revision: f769248

Submitted crossbow builds: ursacomputing/crossbow @ actions-f012880c10

Task	Status
preview-docs

docs/source/cpp/flight.rst

lidavidm · 2022-09-06T20:36:43Z

docs/source/cpp/flight.rst

+gRPC's default behavior allows one server to accept many connections from many
+different clients, but if requests do a lot of work (as they may under Flight),
+the server may not be able to keep up. Configuring the server to limit the number
+of clients and reject requests more proactively, and configuring clients to retry


How do you configure the server to do this?

(IIRC, it's not possible without modifying Flight…)

I think this is referring to gRPC settings.

I would say either remove this text or have a code sample of how to set the server-side limit

docs/source/cpp/flight.rst

lidavidm · 2022-09-06T20:38:24Z

docs/source/cpp/flight.rst

+--------------------------------
+
+1. A stale connection can be closed using
+   :member:`arrow::flight::FlightClientOptions::stop_token`. This requires recording the


This is FlightCallOptions not ClientOptions

I think this is already FlightCallOptions just the diff is off.

I still see ClientOptions here

Saw it here now too. Fixed.

Co-authored-by: David Li <[email protected]>

rok · 2022-09-09T18:49:31Z

Thanks for the review @lidavidm ! I think I addressed everything and sorry for not being faster.

rok · 2022-09-09T19:45:11Z

@github-actions crossbow submit preview-docs

github-actions · 2022-09-09T20:44:28Z

Revision: 0fba44b

Submitted crossbow builds: ursacomputing/crossbow @ actions-20a398e242

Task	Status
preview-docs

rok · 2022-09-13T00:34:36Z

https://crossbow.voltrondata.com/pr_docs/13873/cpp/flight.html#best-practices

lidavidm · 2022-09-13T00:35:27Z

sorry, I'm fairly backed up, I'll try to get to this sometime this week

rok · 2022-09-13T00:43:23Z

No hurry on my end @lidavidm! Sorry for the slow iterations.

docs/source/cpp/flight.rst

lidavidm · 2022-09-15T16:57:19Z

docs/source/cpp/flight.rst

+gRPC's default behavior allows one server to accept many connections from many
+different clients, but if requests do a lot of work (as they may under Flight),
+the server may not be able to keep up. Configuring the server to limit the number
+of clients and reject requests more proactively, and configuring clients to retry


I would say either remove this text or have a code sample of how to set the server-side limit

lidavidm · 2022-09-15T16:57:36Z

docs/source/cpp/flight.rst

+--------------------------------
+
+1. A stale connection can be closed using
+   :member:`arrow::flight::FlightClientOptions::stop_token`. This requires recording the


I still see ClientOptions here

docs/source/cpp/flight.rst

Co-authored-by: David Li <[email protected]>

docs/source/cpp/flight.rst

rok · 2022-09-15T17:35:58Z

@lidavidm thanks for the review! I've merged you change proposals, let me know if I missed something.

Co-authored-by: Rok Mihevc <[email protected]>

lidavidm

Thanks!

lidavidm · 2022-09-15T19:24:02Z

I think in terms of followups:

Can we configure a server to shed load more proactively?
Porting examples to the cookbook
Providing a recipe for a per-read timeout

rok · 2022-09-15T19:50:01Z

@lidavidm shall I open cookbook github issues for those?

lidavidm · 2022-09-15T20:08:50Z

I think you did already but if not, yeah

rok · 2022-09-15T20:20:31Z

Indeed: apache/arrow-cookbook#251

ursabot · 2022-09-16T14:12:26Z

Benchmark runs are scheduled for baseline = e63a13a and contender = 2e72e0a. 2e72e0a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.17% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.28% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.39% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2e72e0a8 ec2-t3-xlarge-us-east-2
[Finished] 2e72e0a8 test-mac-arm
[Failed] 2e72e0a8 ursa-i9-9960x
[Finished] 2e72e0a8 ursa-thinkcentre-m75q
[Finished] e63a13aa ec2-t3-xlarge-us-east-2
[Failed] e63a13aa test-mac-arm
[Failed] e63a13aa ursa-i9-9960x
[Finished] e63a13aa ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

We want to provide best practices and debugging section in: [cpp flight docs](https://arrow.apache.org/docs/cpp/flight.html) [python flight docs](https://arrow.apache.org/docs/python/flight.html) [java flight docs](https://arrow.apache.org/docs/java/flight.html) [R flight docs](https://arrow.apache.org/docs/r/articles/flight.html) Lead-authored-by: Rok <[email protected]> Co-authored-by: Rok Mihevc <[email protected]> Co-authored-by: David Li <[email protected]> Signed-off-by: Rok Mihevc <[email protected]>

github-actions bot added Component: Documentation Component: R labels Aug 14, 2022

lidavidm self-requested a review August 15, 2022 11:33

rok force-pushed the ARROW-17407 branch from ef0c9dd to a5acb74 Compare August 15, 2022 22:34

github-actions bot added the Component: Go label Aug 15, 2022

rok force-pushed the ARROW-17407 branch from a5acb74 to 4640e31 Compare August 16, 2022 00:01

rok marked this pull request as ready for review August 16, 2022 00:02

wjones127 reviewed Aug 17, 2022

View reviewed changes

docs/source/cpp/flight.rst Outdated Show resolved Hide resolved

docs/source/cpp/flight.rst Outdated Show resolved Hide resolved

docs/source/cpp/flight.rst Show resolved Hide resolved

lidavidm reviewed Aug 17, 2022

View reviewed changes

zeroshade reviewed Aug 18, 2022

View reviewed changes

go/arrow/flight/doc.go Show resolved Hide resolved

zeroshade reviewed Aug 18, 2022

View reviewed changes

go/arrow/flight/doc.go Outdated Show resolved Hide resolved

zeroshade requested changes Aug 18, 2022

View reviewed changes

rok commented Aug 18, 2022

View reviewed changes

lidavidm reviewed Sep 6, 2022

View reviewed changes

rok and others added 2 commits September 9, 2022 20:32

Apply suggestions from code review

1092452

Co-authored-by: David Li <[email protected]>

Review feedback

0fba44b

rok closed this Sep 9, 2022

rok reopened this Sep 9, 2022

lidavidm reviewed Sep 15, 2022

View reviewed changes

Apply suggestions from code review

956bb4e

Co-authored-by: David Li <[email protected]>

rok commented Sep 15, 2022

View reviewed changes

docs/source/cpp/flight.rst Outdated Show resolved Hide resolved

Update docs/source/cpp/flight.rst

cfe8f1d

rok commented Sep 15, 2022

View reviewed changes

docs/source/cpp/flight.rst Outdated Show resolved Hide resolved

Update docs/source/cpp/flight.rst

ec64717

Co-authored-by: Rok Mihevc <[email protected]>

lidavidm approved these changes Sep 15, 2022

View reviewed changes

rok merged commit 2e72e0a into apache:master Sep 15, 2022

rok deleted the ARROW-17407 branch September 15, 2022 23:01

		When using default gRPC transport options can be passed to it via
		:member:`arrow::flight::FlightClientOptions::generic_options`. For example:

		// https://issues.apache.org/jira/browse/ARROW-15764 proposes a caching
		// optimisation for server side, but it was not yet implemented.

ARROW-17407: [Doc][FlightRPC] Flight/gRPC best practices #13873

ARROW-17407: [Doc][FlightRPC] Flight/gRPC best practices #13873

Conversation

rok commented Aug 14, 2022

github-actions bot commented Aug 14, 2022

zeroshade commented Aug 14, 2022

rok commented Aug 15, 2022

zeroshade commented Aug 15, 2022

rok commented Aug 15, 2022

rok commented Aug 16, 2022

drin commented Aug 16, 2022

drin commented Aug 16, 2022

lidavidm commented Aug 16, 2022

lidavidm commented Aug 16, 2022

lidavidm commented Aug 16, 2022

drin commented Aug 16, 2022

lidavidm commented Aug 16, 2022

drin commented Aug 16, 2022

lidavidm commented Aug 16, 2022

drin commented Aug 16, 2022 • edited Loading

lidavidm commented Aug 16, 2022

wjones127 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeroshade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rok commented Sep 2, 2022

github-actions bot commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rok commented Sep 9, 2022

rok commented Sep 9, 2022

github-actions bot commented Sep 9, 2022

rok commented Sep 13, 2022

lidavidm commented Sep 13, 2022

rok commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rok commented Sep 15, 2022 • edited Loading

lidavidm left a comment

Choose a reason for hiding this comment

lidavidm commented Sep 15, 2022

rok commented Sep 15, 2022

lidavidm commented Sep 15, 2022

rok commented Sep 15, 2022

ursabot commented Sep 16, 2022

drin commented Aug 16, 2022 •

edited

Loading

rok commented Sep 15, 2022 •

edited

Loading