Overhaul of top-level .md files #15802

jtattermusch · 2018-06-19T13:50:29Z

restructure top-level README.md, INSTALL.md

make "how to use grpc" instructions more accessible, simplify top-level README.md
differentiate between the "use grpc as a dependency" and "develop grpc" workflows
explicitly mention possible build systems for C++, add basic bazel info, improve cmake docs
remove some no-longer-useful info
move the "gRPC concepts" info to a separate doc
fix a number of nits in the documentation (broken links, out-of-date items...)

I'm planning to make a few more changes in a followup PR (improvements for contributing.md, test out the prerequisites list, ...)

grpc-testing · 2018-06-19T13:53:44Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

jtattermusch · 2018-06-19T13:54:37Z

@carl-mastrangelo @hcaseyal @srini100 you all have recently dealt with c-core repo as newcomers. Comments are welcome!

grpc-testing · 2018-06-19T14:02:25Z

[trickle] No significant performance differences

grpc-testing · 2018-06-19T14:44:45Z

[microbenchmarks] No significant performance differences

carl-mastrangelo · 2018-06-19T21:10:59Z

CONCEPTS.md

+## Interface
+
+
+Developers using gRPC typically start with the description of an RPC service


Nit: run on sentence. Would you consider:

Developers using gRPC start with the description of an RPC service (a collection of methods), and generate client and server side interfaces. The server implements the service interface, which can be remotely invoked by the client interface.

+1 for carl's suggestion. Although I'd say to add the "typically" back in if necessary.

carl-mastrangelo · 2018-06-19T21:13:21Z

CONCEPTS.md

+### Surface API
+Starting from an interface definition in a .proto file, gRPC provides
+Protocol Compiler plugins that generate Client- and Server-side APIs.
+gRPC users typically call into these APIs on the Client side and implement


nit: I would make these statements stronger, and thus avoid words like "typically". While technically correct, I would claim that a reader of this page is looking to be told what to do. Once they are more comfortable with gRPC they can branch out.

carl-mastrangelo · 2018-06-19T21:17:24Z

CONCEPTS.md

+fleshing out the details of each of the required operations.
+
+## Abstract gRPC protocol
+A gRPC RPC comprises of a bidirectional stream of messages, initiated by the client. In the client-to-server direction, this stream begins with a mandatory `Call Header`, followed by optional `Initial-Metadata`, followed by zero or more `Payload Messages`. The server-to-client direction contains an optional `Initial-Metadata`, followed by zero or more `Payload Messages` terminated with a mandatory `Status` and optional `Status-Metadata` (a.k.a.,`Trailing-Metadata`).


nit: You used gRPC Call above, and gRPC RPC here. I prefer the former, but I think being consistent is more important.

carl-mastrangelo · 2018-06-19T21:20:02Z

CONCEPTS.md

+The abstract protocol defined above is implemented over [HTTP/2](https://http2.github.io/). gRPC bidirectional streams are mapped to HTTP/2 streams. The contents of `Call Header` and `Initial Metadata` are sent as HTTP/2 headers and subject to HPACK compression. `Payload Messages` are serialized into a byte stream of length prefixed gRPC frames which are then fragmented into HTTP/2 frames at the sender and reassembled at the receiver. `Status` and `Trailing-Metadata` are sent as HTTP/2 trailing headers (a.k.a., trailers).
+
+## Flow Control
+gRPC inherits the flow control mechanisms in HTTP/2 and uses them to enable fine-grained control of the amount of memory used for buffering in-flight messages.


Run on sentence. Perhaps:

gRPC uses the flow control mechanism in HTTP/2. This enables fine-grained control of memory used for buffering in-flight messages.

carl-mastrangelo · 2018-06-19T21:21:29Z

README.md

 ===================================

+gRPC is a modern, open source, high-performance remote procedure call (RPC) framework that can run anywhere. It enables client and server applications to communicate transparently, and makes it easier to build connected systems.
+
+<table>


optional: I believe markdown supports tables.

Leaving as is, this snippet is copied from grpc-java repo: https://raw.githubusercontent.com/grpc/grpc-java/master/README.md

carl-mastrangelo · 2018-06-19T21:23:31Z

README.md

+
+You can find per-language quickstart guides and tutorials in [Documentation section on grpc.io website](https://grpc.io/docs/). The code examples are available in the [examples](examples) directory.
+
+# To start developing gRPC


This section should be in CONTRIBUTING.md right?

Moved the info to CONTRIBUTING.md

carl-mastrangelo · 2018-06-19T21:24:16Z

README.md

+First of all, please refer to the documentation under the `src/YOUR_LANGUAGE` directory for any language-specific instructions on how to build and how to setup your
+development environment.
+
+Different languages use different build systems. To hide the complexity of needing to build with many different build systems, we provide a portable python script that unifies


nit: avoid first-person, unless it's done consistently. I, we, us, etc.

carl-mastrangelo · 2018-06-19T21:24:41Z

README.md

+  instructions
+- Run
+  ```
+  python tools/run_tests/run_tests.py -l YOUR_LANGUAGE --build_only


This needs several dependencies no?

Actually, I was able to use the run_tests.py script for building on a fresh GCE instance (Debian 9) without installing any extra python module (we recently removed the implicit dependency on google-api-client-python).
But I agree in general, let's check if any more dependencies are needed and add them to the docs - I was planning to do one more pass once restructuring the .md files is done.

carl-mastrangelo · 2018-06-19T21:25:38Z

src/cpp/README.md

+- via cmake's `ExternalProject_Add` using a technique called "superbuild". [Example](../../examples/cpp/helloworld/cmake_externalproject/CMakeLists.txt)
+- add gRPC source tree to your project (preferrably as a git submodule) and add it to your cmake project with `add_subdirectory`. [Example](../../examples/cpp/helloworld/CMakeLists.txt)
+
+## bazel


If bazel is recommended, it should be on the top.

hcaseyal

Submitted some preliminary comments, but I will be reviewing the rest of the docs in the near future. Hope they're helpful!

hcaseyal · 2018-06-20T01:05:05Z

BUILDING.md

+From the grpc repository root
+```sh
+ $ make
+ # WARNING: make doesn't provide an easy way to uninstall


Can you provide some context for this warning? If I were someone new to grpc, I may not know why I should be concerned that "make doesn't provide an easy way to uninstall." Is this a situation where if a user needs to be able to easily uninstall grpc, they should use bazel instead?

Follow up q; I thought we do support make uninstall, is that not a sufficient uninstall? Might be work clarifying, because this comment, as is, is a little scary

make uninstall doesn't exist (and to be honest it would be pretty hard to implement, the right solution for uninstalling is to rely on a build system / package manager that has proper support for installing/uninstalling dependencies - I'm hoping we will get there soon).

I agree that more careful wording (and more detailed explanation) would be necessary for the warning. For now, I removed the comment and also the make install line as this document is about how to build (more info on installation is in src/cpp/README.md)

hcaseyal · 2018-06-20T01:08:42Z

BUILDING.md


+# Build from source


Could you provide a brief introduction here that explains why these different build options exist? E.g., when would someone use make vs. bazel? Can bazel be used on all systems? I could envision a new user being confused about which of these options they should choose (or if they need to run a subset of these options).

I agree this would be useful. For now, I provided some basic info about why we support multiple build systems, we can elaborate on this in a followup PR (please see my general comment about merging this PR and iterating).

I think the intro you provided is sufficient 👍 There is a typo though (should be "would work for all...")

hcaseyal · 2018-06-20T01:13:04Z

CONCEPTS.md

+## Interface
+
+
+Developers using gRPC typically start with the description of an RPC service


+1 for carl's suggestion. Although I'd say to add the "typically" back in if necessary.

hcaseyal · 2018-06-20T01:17:01Z

CONCEPTS.md

+and the structure of the payload messages. It is possible to use other
+alternatives if desired.
+
+### Surface API


If I were someone completely new to grpc, I'd have no idea why this section is titled "Surface API." I'd ask, "What is the non-surface API? What is surface?" I'd probably understand eventually, but providing a clearer indication of what is meant by "surface api" would be great. E.g., Surface API (Client- and Server-side APIs).

Looking at this more, I think the problem is in the structure of the topic sentence below. Although the title of this section introduces a potentially new concept, Surface API, the topic sentence begins with other new concepts, and it's not immediately clear how it relates to surface API.

I changed the section title, sg?

grpc-testing · 2018-06-20T10:06:29Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-20T10:14:58Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-20T10:15:24Z

[trickle] No significant performance differences

grpc-testing · 2018-06-20T10:22:59Z

[trickle] No significant performance differences

grpc-testing · 2018-06-20T10:58:21Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-06-20T11:07:44Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-06-20T13:28:01Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-20T13:36:14Z

[trickle] No significant performance differences

grpc-testing · 2018-06-20T14:20:00Z

[microbenchmarks] No significant performance differences

ncteisen

This is awesome

ncteisen · 2018-06-20T14:18:13Z

BUILDING.md

+From the grpc repository root
+```sh
+ $ make
+ # WARNING: make doesn't provide an easy way to uninstall


Follow up q; I thought we do support make uninstall, is that not a sufficient uninstall? Might be work clarifying, because this comment, as is, is a little scary

ncteisen · 2018-06-20T14:19:40Z

BUILDING.md

-> powershell git clone --recursive -b ((New-Object System.Net.WebClient).DownloadString(\"https://grpc.io/release\").Trim()) https://github.com/grpc/grpc
-> cd grpc
-> @rem To update submodules at later time, run "git submodule update --init"
+bazel build :all


Link to our doc or a bazel doc on how to get this dependency?

sg, let's do this in a followup PR? (see my comment #15802 (comment))

ncteisen · 2018-06-20T14:20:42Z

CONCEPTS.md

+
+## Interface
+
+Developers using gRPC start with the description of an RPC service (a collection


.. start with a language agnostic description of an RPC service ...

ncteisen · 2018-06-20T14:22:03Z

CONCEPTS.md

+## Interface
+
+Developers using gRPC start with the description of an RPC service (a collection
+of methods), and generate client and server side interfaces. The server implements


... methods). From this description, gRPC will generate client and server side interfaces in any of our supported languages. The server...

ncteisen · 2018-06-20T14:23:09Z

CONCEPTS.md

+
+gRPC supports streaming semantics, where either the client or the server (or both)
+send a stream of messages on a single RPC call. The most general case is
+Bidirectional Streaming where a single gRPC call establishes a stream where both


nitl change the second where to in which

ncteisen · 2018-06-20T14:24:34Z

CONCEPTS.md

+clients and servers. A concrete embedding over HTTP/2 completes the picture by
+fleshing out the details of each of the required operations.
+
+## Abstract gRPC protocol


At some point this needs its own doc with the same level of detail that PROTOCOL-HTTP2 has...

ncteisen · 2018-06-20T14:25:15Z

CONCEPTS.md

+
+## Implementation over HTTP/2
+The abstract protocol defined above is implemented over [HTTP/2](https://http2.github.io/). gRPC bidirectional streams are mapped to HTTP/2 streams. The contents of `Call Header` and `Initial Metadata` are sent as HTTP/2 headers and subject to HPACK compression. `Payload Messages` are serialized into a byte stream of length prefixed gRPC frames which are then fragmented into HTTP/2 frames at the sender and reassembled at the receiver. `Status` and `Trailing-Metadata` are sent as HTTP/2 trailing headers (a.k.a., trailers).
+


Worth mentioning the http web protocol here?

ncteisen · 2018-06-20T14:26:16Z

README.md

@@ -1,24 +1,79 @@
-[gRPC - An RPC library and framework](http://github.com/grpc/grpc)
+gRPC - An RPC library and framework


nit; I think it would look better if we capitalized the L and F

Will leave as is because grpc-java also uses lowercase.
https://github.com/grpc/grpc-java/blob/3a58a9999e33452268d831da8c9ab61f533380b4/README.md

grpc-testing · 2018-06-20T15:13:03Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-20T15:17:05Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-20T15:23:13Z

[trickle] No significant performance differences

grpc-testing · 2018-06-20T15:25:18Z

[trickle] No significant performance differences

jtattermusch · 2018-06-20T15:33:38Z

Thanks everyone for great comments! I think all of the comments are very useful and I definitely plan to incorporate them, but my worries is that this PR is getting very bloated and the comment thread will be super long.
Here's my suggestion:

let's focus on the overall structure and layout of the docs in this PR. My goal in this PR was to make the important info more visible (e.g. bringing the "how to use" to the front page), get rid of duplication and get to a logical structure that will be easier to maintain / update in the future.
Let's leave wording / non-structural suggestions for follow-up PR. Many of the docs being updated in this PR (like CONCEPTS.md) are just moving a pre-existing text to a different location, but the comments on that text (that was there before without anyone noticing) seem to be piling up. I think it would be good to first agree on the general structure first and then address smaller items in individual .md files later.

Can we get this PR reviewed (and merged) quickly and then continue improving individual .md files separately? That will lead to smaller PRs and better overall productivity IMHO. We will also be able to split the work among more people if we wanted to.
I believe this PR is currently in relatively good shape - it adds some new useful info and almost certainly doesn't make things worse. Let's get it merged and iterate?

@carl-mastrangelo @hcaseyal @ncteisen

grpc-testing · 2018-06-20T15:47:02Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-20T15:56:08Z

[trickle] No significant performance differences

grpc-testing · 2018-06-20T16:04:53Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-06-20T16:08:36Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-06-20T16:38:18Z

[microbenchmarks] No significant performance differences

carl-mastrangelo · 2018-06-20T17:22:12Z

@jtattermusch SGTM

Also LGTM.

jtattermusch · 2018-06-20T18:39:02Z

I squashed the commits and I'm going to merge as soon as the required tests pass.

Remaining comments will be addresses in followup PRs.

grpc-testing · 2018-06-20T18:39:17Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-20T18:46:03Z

[trickle] No significant performance differences

grpc-testing · 2018-06-20T19:30:25Z

[microbenchmarks] No significant performance differences

hcaseyal

Looks good! Left a few more wording suggestions and nits that can be addressed in the other PR

hcaseyal · 2018-06-21T01:03:58Z

BUILDING.md


+# Build from source


I think the intro you provided is sufficient 👍 There is a typo though (should be "would work for all...")

hcaseyal · 2018-06-21T01:13:58Z

CONTRIBUTING.md

+script that unifies the experience of building and testing gRPC in different
+languages and on different platforms is provided.
+
+To build gRPC in the language of choice (e.g. `c++`, `csharp`, `php`, `python`, `ruby`, ...)


Add colon at the end of this line

hcaseyal · 2018-06-21T01:14:10Z

CONTRIBUTING.md

+languages and on different platforms is provided.
+
+To build gRPC in the language of choice (e.g. `c++`, `csharp`, `php`, `python`, `ruby`, ...)
+- Prepare you development environment based on language-specific instructions in `src/YOUR-LANGUAGE` directory.


"you" should be "your"

hcaseyal · 2018-06-21T01:16:13Z

CONTRIBUTING.md

+To build gRPC in the language of choice (e.g. `c++`, `csharp`, `php`, `python`, `ruby`, ...)
+- Prepare you development environment based on language-specific instructions in `src/YOUR-LANGUAGE` directory.
+- The language-specific instructions might involve installing C/C++ prerequisites listed in
+  [Building gRPC C++: Prerequisites](BUILDING.md#pre-requisites) as gRPC implementations


This sentence takes a couple of reads to parse. I would break it up: "The language-specific instructions might involve installing C/C++ prerequisites listed in Building gRPC C++: Prerequisites. This is because gRPC implementations in this repository are using the native gRPC "core" library internally."

hcaseyal · 2018-06-21T01:17:07Z

CONTRIBUTING.md

+  ```
+
+You can also run `python tools/run_tests/run_tests.py --help` to discover useful command line flags supported. For more details,
+see [tools/run_tests](tools/run_tests) where you will also find guidance on how to run various other test suites (e.g. interop tests, benchmarks)


Period missing at the end of this sentence.

jtattermusch assigned srini100, carl-mastrangelo, hcaseyal and dgquintas Jun 19, 2018

carl-mastrangelo reviewed Jun 19, 2018

View reviewed changes

hcaseyal reviewed Jun 20, 2018

View reviewed changes

ncteisen reviewed Jun 20, 2018

View reviewed changes

carl-mastrangelo approved these changes Jun 20, 2018

View reviewed changes

overhaul of top-level .md files

f389e52

jtattermusch force-pushed the contributing_improvements branch from 3c81708 to f389e52 Compare June 20, 2018 18:35

jtattermusch merged commit fd7683c into grpc:master Jun 20, 2018

This was referenced Jun 20, 2018

Improve CONCEPTS.md wording #15821

Merged

Improve BUILDING.md and CONTRIBUTING.md #15822

Merged

Change INSTALL.md for BUILDING.md after release grpc/grpc.github.io#692

Open

hcaseyal reviewed Jun 21, 2018

View reviewed changes

jtattermusch added the release notes: no Indicates if PR should not be in release notes label Jul 19, 2018

This was referenced Aug 13, 2018

Get rid of references to INSTALL.md grpc/grpc.github.io#723

Merged

Update INSTALL.md for Cross Compile Instructions #11767

Closed

fix headers in INSTALL.md for gfm compatibility #10215

Closed

docs : add prerequisites for osx user #15181

Closed

lock bot locked as resolved and limited conversation to collaborators Oct 17, 2018

		## Interface


		Developers using gRPC typically start with the description of an RPC service


		You can find per-language quickstart guides and tutorials in [Documentation section on grpc.io website](https://grpc.io/docs/). The code examples are available in the [examples](examples) directory.

		# To start developing gRPC


		## Interface

		Developers using gRPC start with the description of an RPC service (a collection


		## Implementation over HTTP/2
		The abstract protocol defined above is implemented over [HTTP/2](https://http2.github.io/). gRPC bidirectional streams are mapped to HTTP/2 streams. The contents of `Call Header` and `Initial Metadata` are sent as HTTP/2 headers and subject to HPACK compression. `Payload Messages` are serialized into a byte stream of length prefixed gRPC frames which are then fragmented into HTTP/2 frames at the sender and reassembled at the receiver. `Status` and `Trailing-Metadata` are sent as HTTP/2 trailing headers (a.k.a., trailers).

		@@ -1,24 +1,79 @@
		[gRPC - An RPC library and framework](http://github.com/grpc/grpc)
		gRPC - An RPC library and framework

Overhaul of top-level .md files #15802

Overhaul of top-level .md files #15802

Conversation

jtattermusch commented Jun 19, 2018

grpc-testing commented Jun 19, 2018

jtattermusch commented Jun 19, 2018

grpc-testing commented Jun 19, 2018

grpc-testing commented Jun 19, 2018

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

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

hcaseyal 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

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

ncteisen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch Jun 20, 2018 • edited Loading

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

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

jtattermusch commented Jun 20, 2018 • edited Loading

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

carl-mastrangelo commented Jun 20, 2018

jtattermusch commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

grpc-testing commented Jun 20, 2018

hcaseyal 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

jtattermusch Jun 20, 2018 •

edited

Loading

jtattermusch commented Jun 20, 2018 •

edited

Loading