MovR tutorial for SQLAlchemy + Flask

[ericharmeling]

Fixes #6056.

This PR contains the entire end-to-end guide for the movr-flask application.

I've split up the guide into six parts:

1. Developing and Deploying a Multi-Region Web Application (multi-region-overview.md)
2. MovR: An Example Multi-Region Use-Case (multi-region-use-case.md)
3. Creating a Multi-Region Database Schema (multi-region-database.md)
4. Setting Up a Virtual Environment for Developing Multi-Region Applications (multi-region-setup.md)
5. Developing a Multi-Region Web Application (multi-region-application.md)
6. Deploying a Multi-Region Web Application (multi-region-deployment.md)

I'm assigning each reviewer on here to a subset of the files, but a complete review is always welcome.

@jseldess:

• multi-region-overview.md
• multi-region-use-case.md
• multi-region-database.md
• multi-region-deployment.md (writer review - these instructions are pretty much identical to the instructions in the movr-flask README)

@rmloveland:

• multi-region-setup.md
• multi-region-application.md

@ajwerner:

• multi-region-deployment.md (technical review)

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/30b93f789a2860703fcc1b5743184698ba4482ee/

Edited pages:

• v19.2/tutorials-sqlalchemy-movr.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/08d6fe31c0560e816df7673ff687b2a7fc1cae88/

Edited pages:

• v19.2/tutorials-sqlalchemy-movr.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/118f7bac7e7e60f6d3744547eea246d6b3276e78/

Edited pages:

• v19.2/tutorials-sqlalchemy-movr.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/bd4f8a52f278f14e88c850cbd3e8d6b104e39603/

Edited pages:

• v19.2/tutorials-sqlalchemy-movr.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/8b420ccf1ce53326a13bec56fa3d5bb3707c87f8/

Edited pages:

• v19.2/tutorials-sqlalchemy-movr.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/45b81517178e859377270f0832b10573b5b608cd/

Edited pages:

• v19.2/tutorials/multi-region-flask.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/7ccb876430c83b2ae17dc9c1cf39cd98abdbb982/

Edited pages:

• v19.2/tutorials/multi-region-flask.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/18ccb463315703edf26a164b08ab6c3f77508a3b/

Edited pages:

• v19.2/tutorials/multi-region-flask.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/b4d743136d78e491cbb0b73e8d36749877456e88/

Edited pages:

• v19.2/tutorials/multi-region-flask.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/c22be2b91a74f491a1565e1647895bf8ffe89f1b/

Edited pages:

• v19.2/tutorials/multi-region-flask.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/a5d388de47366d0f624391eda6abec59ee965342/

Edited pages:

• v19.2/tutorials/multi-region-application.md
• v19.2/tutorials/multi-region-database.md
• v19.2/tutorials/multi-region-deployment.md
• v19.2/tutorials/multi-region-overview.md
• v19.2/tutorials/multi-region-setup.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/3541d0e9bc29d8640d0ea15027f580090674b13e/

Edited pages:

• v19.2/tutorials/multi-region-application.md
• v19.2/tutorials/multi-region-database.md
• v19.2/tutorials/multi-region-deployment.md
• v19.2/tutorials/multi-region-overview.md
• v19.2/tutorials/multi-region-setup.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/fb535e075e4227021a587c243f7b49fbbf46d03f/

Edited pages:

• v19.2/tutorials/multi-region-application.md
• v19.2/tutorials/multi-region-database.md
• v19.2/tutorials/multi-region-deployment.md
• v19.2/tutorials/multi-region-overview.md
• v19.2/tutorials/multi-region-setup.md
• v19.2/tutorials/multi-region-use-case.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/3029cbbf0f06d8adb2cf8d8a2ff360c0ec3dc088/

Edited pages:

• v19.2/tutorials/multi-region-application.md
• v19.2/tutorials/multi-region-database.md
• v19.2/tutorials/multi-region-deployment.md
• v19.2/tutorials/multi-region-overview.md
• v19.2/tutorials/multi-region-setup.md
• v19.2/tutorials/multi-region-use-case.md

[rmloveland]

This is great Eric! I had a lot of feedback, edit requests, and questions but I love it overall! Thanks for writing this.

[rmloveland]

Consider de-gerundificating this to "Develop a multi-region web application"?

[rmloveland]

I think it would be good to follow the structure we have in a number of other page by making a "Before you begin" section, which would allow people who landed on this page somehow (such as via search) to more easily figure out "oh hey, I need to go do that other page first". Also, familiarity of structure makes it easier to consume info.

[rmloveland]

Suggest something like "consists of the following components, each of which is described in a separate section below". Idea being to give the user a more explicit mind map of what is on this page before diving in.

[rmloveland]

Same comment as elsewhere re: referring to demo clusters as "virtual". I think we should not overload this term and stick with "demo cluster" which we already use elsewhere.

[rmloveland]

Each element in this list of stuff could also be formatted in reverse order, as e.g., 'Building the web application: A Flask server that handles requests from mobile apps and browsers'. This might make it a little clearer to read (IMO).

It's also confusing that some of these links are to another page, and some are to headings on this page. Suggest splitting them out and being explicit about what lives where to orient the reader.

[rmloveland]

This exact prompt string probably only applies on macOS, which uses an ancient pre-GPLv3 bash for legal reasons. Also, not everyone uses bash (I don't). Perhaps this could say something general like "the prompt should change to your shell's default prompt. This signals that you are now in the Python virtual environment"?

[rmloveland]

Recommend removing "simply" per style guide.

[rmloveland]

Nit: Is this mention of gunicorn necessary for getting me through this tutorial?

[rmloveland]

Regarding the previous comment, if we did not have the gunicorn option listed above, could we state a URL explicitly here? I.e., whatever the default URL used by python3 server.py results in? That would be easier (and clickable).

[rmloveland]

Multiple uses of "develop and debug". You could say something like "now that your environment is set up, you can start developing and debugging your app ...".

Also, I know some pages do not have this, but I'm a big fan of every page having a See Also section with links to pages that have relevant content.

[ericharmeling]

@rmloveland Thank you for the very thorough review, Rich. Immensely helpful! I made a ton of changes to work in your feedback. :))

I'll squash that commit before merging, after all reviews are complete.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jseldess, and @rmloveland)

---

v19.2/tutorials/multi-region-application.md, line 3 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Consider de-gerundificating this to "Develop a multi-region web application"?

Fair enough. This is admittedly a remnant of my former company's style guide. It makes sense to make it more consistent with the rest of the CRDB docs.

---

v19.2/tutorials/multi-region-application.md, line 7 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

I think it would be good to follow the structure we have in a number of other page by making a "Before you begin" section, which would allow people who landed on this page somehow (such as via search) to more easily figure out "oh hey, I need to go do that other page first". Also, familiarity of structure makes it easier to consume info.

Great idea. I'm going to lead with a general description of the page, and then mention that it is part of a larger guide. After that, I'll make a "Before you begin" section and add reading the previous section to the list of requirements.

---

v19.2/tutorials/multi-region-application.md, line 11 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Suggest something like "consists of the following components, each of which is described in a separate section below". Idea being to give the user a more explicit mind map of what is on this page before diving in.

Good point. Not all of these are described in a section "below". I'll just say " each of which is described in a separate section".

---

v19.2/tutorials/multi-region-application.md, line 14 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same comment as elsewhere re: referring to demo clusters as "virtual". I think we should not overload this term and stick with "demo cluster" which we already use elsewhere.

Done.

---

v19.2/tutorials/multi-region-application.md, line 19 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Each element in this list of stuff could also be formatted in reverse order, as e.g., 'Building the web application: A Flask server that handles requests from mobile apps and browsers'. This might make it a little clearer to read (IMO).

It's also confusing that some of these links are to another page, and some are to headings on this page. Suggest splitting them out and being explicit about what lives where to orient the reader.

I agree that we can be more explicit about where these sections live. I don't know if it makes a lot of sense reordering the list though. The sections aren't the components, which is how this list is laid out. I've new-lined the sections so that they are a little cleaner.

---

v19.2/tutorials/multi-region-application.md, line 63 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Suggest moving this up with the sentence 'Here is the .. directory structure' before the long code block. Could get a little lost here between the big code block and the next heading.

Done.

---

v19.2/tutorials/multi-region-application.md, line 67 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Typo: ORMs (plural)

Done.

---

v19.2/tutorials/multi-region-application.md, line 69 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Elsewhere in our docs we refer to cockroachdb-python as the SQLAlchemy "CockroachDB dialect". They are referred to as dialects in the SQLAlchemy parlance: https://docs.sqlalchemy.org/en/13/dialects/

I would argue that it's more than a SQLAlchemy dialect, but we can refer to it as that for this guide.

---

v19.2/tutorials/multi-region-application.md, line 71 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

General comment re: headings everywhere - consider de-gerundificating? E.g. this would become the imperative 'Declare mappings'

Done.
As mentioned before, this is a remnant of a style guide at another company I worked for........

---

v19.2/tutorials/multi-region-application.md, line 73 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Suggest making "movr" be a link to the page describing movr just in case the reader is not familiar and needs a quick refresher here.

Also, it would be good to edit this so it makes less assumptions about what the user knows or doesn't know. This overall "guide" has a continuity in your mind, but that won't necessarily hold for someone landing on this page from a search.

Perhaps something like "Based on working through Multi-region-setup, you should be familiar with ...". Also another argument for creating a "Before you Begin" section above.

I agree. I think adding the Before you begin sections seriously improved this.
Just added a bit introducing this, pointing to that previous section.

---

v19.2/tutorials/multi-region-application.md, line 93 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

When linking to another heading on the same page, I think it's a good practice to make that clear to the reader in the text before they click the link, e.g., "We'll cover ... more detail in the Building a foo ... section below."

Whenever I see a link I wonder "is this going to take me to some other page altogether? or another section of this doc?"

Good point. Done.

---

v19.2/tutorials/multi-region-application.md, line 121 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Suggest "we will use" and avoiding "just" (per Style Guide).

Done.

---

v19.2/tutorials/multi-region-application.md, line 178 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

It would be more idiomatic to refer to this as the "SQLAlchemy CockroachDB dialect" for reasons mentioned in another comment.

Done.

---

v19.2/tutorials/multi-region-application.md, line 180 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Might be worth showing a bit of pseudocode here to go with this prose describing run_transaction(), IMO. Could be a one-liner. It's easy to understand when you see it - probably easier than reading about it, depending on your POV.

Here is a possible example of actual code (not a real function defined in this application, but syntactically correct):

"
For example, if you are writing a function that updates a single row, you would need to call run_transaction():

def update_row(id, value):
    def update_transaction_callback(session, id, value):
        row = session.Query(Table).filter(id == id)
        row.key = value
    return run_transaction(sessionmaker(bind=engine), lambda session: update_transaction_callback(session, id, value))

"

I feel like because there are a lot of moving parts here. You need an Engine defined, a Table defined, a transaction callback function defined, then you have to use only a subset of SQLAlchemy functions. This sample could be more confusing than clarifying. Especially since we go over several examples of its proper usage in the context of the example application in a section just below this. I do this its a fair point though that we need to show some examples, so I added a sentence after the exposition that points to the example usages.

---

v19.2/tutorials/multi-region-application.md, line 182 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

We should replace the "transaction retries are more frequent in CockroachDB" sentence IMO. I think it's a slightly unflattering framing and may be technically untrue in some sense. For example, if you turn up the isolation level in Postgres beyond the default ("read committed" I think), my understanding is that you will also start seeing serializability retries on many workloads.

Suggest re-using the language from the transactions doc:

Transactions may require retries if they experience deadlock or read/write contention with other concurrent transactions which cannot be resolved without allowing potential serializable anomalies

Done.

---

v19.2/tutorials/multi-region-application.md, line 186 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Suggest rewriting to avoid the parenthetical '(not a session)'. If it's important we should use the word "must", which carries specific connotation in e.g. RFCs. Maybe something like "When passed a FooMaker, it ensures that A, B, C ... Note that you must pass a FooMaker, not a Foo, because X, Y, Z".

Done, with some clarifying language. You don't have to pass sessionmaker to run_transaction(). As explained in the first paragraph of this section, you can also pass Engine or Connection as a "transactor". Under no circumstances can you pass session.

---

v19.2/tutorials/multi-region-application.md, line 187 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

We explained this point above. May be worth collapsing this and the previous section together.

I actually think this is offering new information about application portability. I'll simplify the wording to make that clearer.

---

v19.2/tutorials/multi-region-application.md, line 188 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

It's not necessarily true that it will run "much faster than a naive implementation". There was some discussion and testing in this mailing list thread (see also the linked GH issue).

TL;DR: app retry loops that do not use the SAVEPOINT dance are basically fine most of the time, and can perform well if you add exponential backoff.

I just looked for this language re: running to completion faster, etc. and I wrote it (LOL, sorry). I filed #6390 to fix / update that! We also have the related issue #6261 in the 20.1 cycle

lol OK. I'm just going to remove this bullet point then, as it doesn't seem to apply in all cases.

---

v19.2/tutorials/multi-region-application.md, line 198 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

This could drop "since that will lead to breakage", since the cases / behaviors (breakage?) appear to be explained directly below in the following list.

Done.

---

v19.2/tutorials/multi-region-application.md, line 202 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

We could also add a link to the SAVEPOINT doc here, which notes this limitation. Strangely it's not on the known limitations page.

Done.

---

v19.2/tutorials/multi-region-application.md, line 211 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Is this the same recommendation as above re: the sessionmaker vs. session? Also, I can't remember why it needs to be a Sessionmaker? (I remember this from working on the SQLAlchemy docs, but not why) If it's simple to say why it matters, we should.

Also: do we "recommend" it? Or is it the case that "you must pass a session maker object"? (boldness mine, not necessarily recommending that for the text).

If the latter, we should state that.

Actually, this is different. Here we are making an explicit recommendation to use sessionmaker over the other possible options for transactor arguments passed to run_transaction(). sessions is not an allowable type for a transactor. I made this much more explicit.

---

v19.2/tutorials/multi-region-application.md, line 231 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Question: is it necessary (or idiomatic) that the functions be named get_foo_txn()? or could they be called get_foo()? Asking because "txn" is a bit repetitive, and if I were writing the app, I'd like the function to abstract away the data store and the fact that there is something called "transactions" happening in the backend, and just do the job of "give me the rides taken by this rider".

(I wrote this before we chatted yesterday afternoon re: which functions are helper functions. I'm leaving it because I think it feeds into the discussion of whether the public namespace of this API needs to have these helper functions exported into it, or not).

I think it does help separate the the callbacks from the functions that the routes point to. When I first wrote the application, these were all suffixed with _callback, but it was suggested that I change the name to _txn for brevity (and clarity?).

---

v19.2/tutorials/multi-region-application.md, line 236 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Would it be clearer to say "this function gets all the rides for a specific rider across multiple regions. It does not filter the rides by city" (or something like that)?

Also: it would be good to know explicitly why you are telling me this information. Are you saying this is good? Or bad?

I added a much longer explanation of why this is bad, but not as bad as it could be because this is a read operation, and not a write operation.

---

v19.2/tutorials/multi-region-application.md, line 242 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Dumb movr question: do cities have IDs? I'd expect the idiom here to use a city ID, since above we used a rider ID.

(Update after our discussion yesterday: apparently cities do not have IDs, just string names. I think this is a limitation that could lead to errors by someone writing an application IRL, since there is no argument checking at the function boundary, it's just a randomly selected string variation of the city's name.)

I'm not sure I really understand what you mean entirely. Would love to talk more about this issue offline sometime. Definitely a code issue though.

---

v19.2/tutorials/multi-region-application.md, line 244 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Nit: Is there a (non-bad) way to reduce the line length here to be PEP-8 compliant? flake8 complains about these long dictionary lines.

My VScode formatter is all kinds of messed up. I just formatted all the python files to make them pep8-compliant.

---

v19.2/tutorials/multi-region-application.md, line 247 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Similar to the above: Are you telling me this because this is a good thing that I want to happen? Why is it good (or not)?

Done.

---

v19.2/tutorials/multi-region-application.md, line 256 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Why do I need to pass both a city and a rider city? Presumably for a ride to start the rider must be in the same city where the ride starts? Would these values ever NOT be the same?

Actually, these values can be different. A rider is registered in a particular city, and they can only create vehicles in that city. But they can go to other cities and start rides there. So rider_city could be new york (where I am registered as a user), and then if I go to Seattle and start a ride there, city (i.e. "ride city") would be seattle.

---

v19.2/tutorials/multi-region-application.md, line 278 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same comment as above re: long lines make flake8 complain.

Done.

---

v19.2/tutorials/multi-region-application.md, line 307 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same comment as earlier re: passing city and rider_city - shouldn't these be the same at ride start time? Would they ever not?

See my response above. The ride city and the rider_city can be different.

---

v19.2/tutorials/multi-region-application.md, line 308 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Ah so this is why the foo_txn() functions exist, right? They are always passed as arguments to run_transaction()?

If these are always helper functions, could (for instance) the definition of start_ride_txn() be moved inside start_ride()? Is start_ride() the only caller?

If so I would have these as external functions while I was writing & debugging them, but would move them inside the start_ride() function once they were working properly.

I have no idea if what I just described is idiomatic Python, but it's common in some other languages like Lisp/Scheme.

Yes! These are the transaction callback functions that we've been talking about up to this point. They are passed to run_transaction().

So, Nate's implementation of the MovR backend (https://github.com/cockroachdb/movr/blob/master/movr.py), puts these callbacks within the API function definitions, as you suggest. Per our brief discussion offline, I defined my transaction functions in a separate file, so it would be easier to talk about the callbacks and look at them as their own, separate functions. I think it just makes each file (movr.py and transactions.py) cleaner and easier to document.

As far as it being idiomatic, I'm not sure I have a good answer for this. I've definitely seen callback functions defined in a separate file in other Python projects, especially if the callbacks are pretty complex. I'm of the (possibly erroneous) opinion that it is a matter of preference and doesn't really matter. Also, none of the engineers who reviewed this who have had production Python experience said it was an issue.

Anyways, all of this is more about the code. I think we should look over in the code repo itself: https://github.com/cockroachlabs/movr-flask.

---

v19.2/tutorials/multi-region-application.md, line 311 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

This is a prose description of what is happening in the code. But it might be helpful to add some "why" here in addition to the "what"? I think this is a general comment about all the places in this guide where the operation of a snippet of code is verbally described.

(PS I also do this, I'm probably only noticing it because I'm the reviewer on this one :-} )

I've tried to add a little more to this description, to explain the why. Things can get pretty existential pretty quickly...

---

v19.2/tutorials/multi-region-application.md, line 313 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Further bolstering the argument that start_ride_txn() could be an internal helper function?

We can discuss this offline, if need-be. See my comment above.

---

v19.2/tutorials/multi-region-application.md, line 315 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Might as well link to this file if you want me to review it.

Done.

---

v19.2/tutorials/multi-region-application.md, line 319 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Also: Suggest saying something more neutral like "The application needs a front end / user interface" or similar instead of "all it needs now", as that could be interpreted as it's "just" a user interface (making good user interfaces is not easy).

I just replaced the first part, with "The application needs a frontend and a user interface."

---

v19.2/tutorials/multi-region-application.md, line 341 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same as elsewhere re: preferring our existing language "demo cluster" over "virtual cluster", which (AFAICT?) is only in this PR. (Please correct me if I'm wrong, though!)

This could be re-ordered as "The application sets a DEBUG config variable, which it checks to determine whether to ...", which would be clearer IMO.

Done.

---

v19.2/tutorials/multi-region-application.md, line 360 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

In general any mention of a file such as movr.py should be a link to that file. It only takes us a second but saves the reader so much time searching manually.

Done.

---

v19.2/tutorials/multi-region-application.md, line 410 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

I think we should add a note below this snippet that HTTP is not secure for production deployments of an application that will be accepting user passwords

Done.

---

v19.2/tutorials/multi-region-application.md, line 417 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

We should link to WSGI if we are going to mention it. I happen to know what it is but some people may not.

In fact, do we need to mention WSGI at all? If you don't already know what it is then you probably don't need to care to get through this tutorial, right?

Good point. Removed it.

---

v19.2/tutorials/multi-region-application.md, line 424 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

I think all mentions of Flask objects (such as Config here, right?) should be links to the docs for those object types.

I think this para could be improved by a sentence that says what is happening at a higher level (e.g, "this code establishes a database connection"?) before diving into the details of constructors and such.

Done.

---

v19.2/tutorials/multi-region-application.md, line 440 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same as above re: can LoginManager be a link to API doc?

Done.

---

v19.2/tutorials/multi-region-application.md, line 446 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Plural nit: "URLs"

Done.

---

v19.2/tutorials/multi-region-application.md, line 449 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Can these methods have links to the Flask API docs for these methods?

Done.

---

v19.2/tutorials/multi-region-application.md, line 453 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Link to movr.py file?

Done.

---

v19.2/tutorials/multi-region-application.md, line 455 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Unnecessary extra comma after "look".

Done.

---

v19.2/tutorials/multi-region-application.md, line 482 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Another thought: Is this aside even needed? I think the next paragraph is also a perfectly good opener.

Removed the first sentence.

---

v19.2/tutorials/multi-region-application.md, line 486 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

I think this and all other mentions of "this section of the guide" everywhere should be changed to "for the purposes of this document". The majority of readers are probably finding a docs page via search, not reading a guide in order. I don't think anybody but us is thinking so explicitly about "guides" (maybe I'm wrong?).

Typo: did you mean "where the client IS located" ?

That is an excellent point. I've tried to clean up everything to make it a little easier for people coming to the page from a google search or something.

Fixed the typo.

---

v19.2/tutorials/multi-region-application.md, line 505 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

This comment is pretty long, and will scroll off the page to the right. Maybe fill to 72 columns? Especially since these instructions seem important.

Removed it.

---

v19.2/tutorials/multi-region-application.md, line 515 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Suggest using "demo cluster".

Could scratch the "perhaps the first thing you'll notice" intro and start with "The function's control flow sequence ..." without loss of clarity.

Suggest removal of adjectives "greatly" and "just" (latter per style guide).

Done.

---

v19.2/tutorials/multi-region-application.md, line 517 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Suggest simplifying the verbal logic to something like "If the application is not in DEBUG mode, then ..." to match the phrasing in the previous paragraph for easier reading. Also if .. then structure more closely matches code logic flow, I think.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 2 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Maybe the imperative "Set up a virtual environment ..."

Done.

---

v19.2/tutorials/multi-region-setup.md, line 7 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Suggest calling this a "demo cluster" to match our usage/description in the CLI docs and elsewhere.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 13 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Meta: I don't think we should nest the pages of this tutorial in a directory. It makes things a little easier while doing the initial drafting, perhaps, but leads to having to type ../page.html a lot forever after (and it's easy to forget to do so).

It also makes writing tools that do things to our docs (such as convert to PDF, etc.) harder to write, since they cannot assume a directory full of only HTML files.

If I could I would go back in time and try not have an architecture/ directory, for example.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 15 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Pipenv link appears to be down now? However this worked: https://pipenv.readthedocs.io/en/latest/

Done.

---

v19.2/tutorials/multi-region-setup.md, line 17 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Hmm, I was confused here. Do you mean: don't worry about it yet, you'll give me a requirements.txt later? The way it's worded makes me think I have to care now.

If I don't have to care now, perhaps we should remove this note and just let the user encounter the directions for installing the Python libs later when it's needed?

That's a much better move. I removed the note.

---

v19.2/tutorials/multi-region-setup.md, line 19 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Rather than saying "the required tools and applications", I recommend being more explicit and saying "installing the prerequisites listed above" or some such. That way I know whether I have installed what you want me to have installed at this point in time (or not).

Done.

---

v19.2/tutorials/multi-region-setup.md, line 21 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same as elsewhere re: preferring the existing "demo cluster" to the new (to me at least) "virtual cluster"

Done.

---

v19.2/tutorials/multi-region-setup.md, line 23 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Also: This paragraph seems to open with the caveat, then state what it wants me to actually do. Recommend reversing the order: tell me what to do, then mention the caveat about how this is a development-only workflow.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 25 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same as above re: replacing "virtual" with "demo cluster" or similar.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 27 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

This step's description could pull in the "to set up the cluster" phrase above it, e.g., "1. To set up the multi-region demo cluster, run cockroach demo as shown below. Something something flags, localities, etc.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 37 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Suggest just using the actual port number from your machine here. It is easier to edit that in terms of keystrokes (IMO) than editing <some_port>.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 40 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same as above re: "virtual"

Done.

---

v19.2/tutorials/multi-region-setup.md, line 42 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same as above re: <some_port>.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 44 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Recommend being more explicit, e.g., "Open another terminal window. In the new window ..."

Done.

---

v19.2/tutorials/multi-region-setup.md, line 48 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same as above re: <some_port>.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 51 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Recommend moving this to the instruction text above, so I know what I'm loading in before I do it.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 53 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Do you mean "In the terminal window you opened before"? If so, I suggest numbering them and referring to them as explicitly as Terminal Window #1 and Terminal Window #2, etc., so there can be no ambiguity.

Just updated this to read "the demo cluster terminal", which is much better than what I had before, and fairly unambiguous since in just a couple steps before we instruct the user to leave this terminal open.

---

v19.2/tutorials/multi-region-setup.md, line 71 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

This is confusing. I think you are ultimately telling me to not use k8s in this example since we are going to use pipenv on the local machine, right?

If so (?), I recommend telling me what to do first, and adding any caveats to a separate note that comes after the explicit instructions. I would prefer a separate caveat note after the explicit instructions, so I don't mix them together in my mind.

That's a fair point. I removed that caveat from the intro of this section and added a note at the end of these steps.

---

v19.2/tutorials/multi-region-setup.md, line 116 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Recommend linking "connection string" to our Connection parameters reference page

Done.

---

v19.2/tutorials/multi-region-setup.md, line 118 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Recommend dropping "so" and going with "Open .env and edit the DB_URI ...".

Recommend dropping "just" (see style guide). For this sentence, you could say something like "you may need to change the port number".

Recommend "Note that SQLAlchemy requires ... as shown below", so it's clear you are about to give me the magic string.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 126 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Recommend dropping this if it is not relevant to me getting through this tutorial. You could replace it with something like "For more information about the features of the .env file format, see the pipenv docs".

The .env provided has some variables in it that you can and should use in production, but not in this local deployment set up. So if they open up the .env file, there will be several variables defined there that might be a little confusing. I reworded this to make it a little clearer.

---

v19.2/tutorials/multi-region-setup.md, line 136 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

This exact prompt string probably only applies on macOS, which uses an ancient pre-GPLv3 bash for legal reasons. Also, not everyone uses bash (I don't). Perhaps this could say something general like "the prompt should change to your shell's default prompt. This signals that you are now in the Python virtual environment"?

I'm just going to remove that first sentence.

---

v19.2/tutorials/multi-region-setup.md, line 138 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Recommend removing "simply" per style guide.

Done.

---

v19.2/tutorials/multi-region-setup.md, line 145 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Nit: Is this mention of gunicorn necessary for getting me through this tutorial?

No. I removed it. It's used in the production deployment.

---

v19.2/tutorials/multi-region-setup.md, line 152 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Regarding the previous comment, if we did not have the gunicorn option listed above, could we state a URL explicitly here? I.e., whatever the default URL used by python3 server.py results in? That would be easier (and clickable).

Done.

---

v19.2/tutorials/multi-region-setup.md, line 157 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Multiple uses of "develop and debug". You could say something like "now that your environment is set up, you can start developing and debugging your app ...".

Also, I know some pages do not have this, but I'm a big fan of every page having a See Also section with links to pages that have relevant content.

Done.

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/06ae23f52e3f3c3b6b93f24e0fa7eae8c02f47d7/

Edited pages:

• v19.2/multi-region-application.md
• v19.2/multi-region-database.md
• v19.2/multi-region-deployment.md
• v19.2/multi-region-overview.md
• v19.2/multi-region-setup.md
• v19.2/multi-region-use-case.md

[rmloveland]

Anytime, Eric! Thanks for the updates - LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ericharmeling, @jseldess, and @rmloveland)

---

v19.2/tutorials/multi-region-application.md, line 19 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

I agree that we can be more explicit about where these sections live. I don't know if it makes a lot of sense reordering the list though. The sections aren't the components, which is how this list is laid out. I've new-lined the sections so that they are a little cleaner.

Works for me - thanks!

---

v19.2/tutorials/multi-region-application.md, line 180 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

Here is a possible example of actual code (not a real function defined in this application, but syntactically correct):

"
For example, if you are writing a function that updates a single row, you would need to call run_transaction():

def update_row(id, value):
    def update_transaction_callback(session, id, value):
        row = session.Query(Table).filter(id == id)
        row.key = value
    return run_transaction(sessionmaker(bind=engine), lambda session: update_transaction_callback(session, id, value))

"

I feel like because there are a lot of moving parts here. You need an Engine defined, a Table defined, a transaction callback function defined, then you have to use only a subset of SQLAlchemy functions. This sample could be more confusing than clarifying. Especially since we go over several examples of its proper usage in the context of the example application in a section just below this. I do this its a fair point though that we need to show some examples, so I added a sentence after the exposition that points to the example usages.

Fair! That's a lot of code to try to wedge in here, as you say.

---

v19.2/tutorials/multi-region-application.md, line 186 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

Done, with some clarifying language. You don't have to pass sessionmaker to run_transaction(). As explained in the first paragraph of this section, you can also pass Engine or Connection as a "transactor". Under no circumstances can you pass session.

Ah ok - thanks! In any case I trust your knowledge here :-)

---

v19.2/tutorials/multi-region-application.md, line 211 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

Actually, this is different. Here we are making an explicit recommendation to use sessionmaker over the other possible options for transactor arguments passed to run_transaction(). sessions is not an allowable type for a transactor. I made this much more explicit.

Ah, thanks for explaining!

---

v19.2/tutorials/multi-region-application.md, line 256 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

Actually, these values can be different. A rider is registered in a particular city, and they can only create vehicles in that city. But they can go to other cities and start rides there. So rider_city could be new york (where I am registered as a user), and then if I go to Seattle and start a ride there, city (i.e. "ride city") would be seattle.

Ah, ok! Thanks for explaining.

---

v19.2/tutorials/multi-region-application.md, line 308 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

Yes! These are the transaction callback functions that we've been talking about up to this point. They are passed to run_transaction().

So, Nate's implementation of the MovR backend (https://github.com/cockroachdb/movr/blob/master/movr.py), puts these callbacks within the API function definitions, as you suggest. Per our brief discussion offline, I defined my transaction functions in a separate file, so it would be easier to talk about the callbacks and look at them as their own, separate functions. I think it just makes each file (movr.py and transactions.py) cleaner and easier to document.

As far as it being idiomatic, I'm not sure I have a good answer for this. I've definitely seen callback functions defined in a separate file in other Python projects, especially if the callbacks are pretty complex. I'm of the (possibly erroneous) opinion that it is a matter of preference and doesn't really matter. Also, none of the engineers who reviewed this who have had production Python experience said it was an issue.

Anyways, all of this is more about the code. I think we should look over in the code repo itself: https://github.com/cockroachlabs/movr-flask.

Cool, thanks for explaining. Defer to you and your code reviewers here.

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/cf25f154129fc6470f355c1190b2eca672ffca1e/

Edited pages:

• v19.2/multi-region-application.md
• v19.2/multi-region-database.md
• v19.2/multi-region-deployment.md
• v19.2/multi-region-overview.md
• v19.2/multi-region-setup.md
• v19.2/multi-region-use-case.md

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/cf25f154129fc6470f355c1190b2eca672ffca1e/

Edited pages:

• v19.2/multi-region-application.md
• v19.2/multi-region-database.md
• v19.2/multi-region-deployment.md
• v19.2/multi-region-overview.md
• v19.2/multi-region-setup.md
• v19.2/multi-region-use-case.md

[jseldess]

This is really great, @ericharmeling. Sorry for taking so long to get you my review. LGTM, with nits here and there. Long-term, I think there are opportunities to enhance the feeling of journey and progress, and I think we haven't done enough to call out best practices specific to CockroachDB (https://www.cockroachlabs.com/docs/stable/performance-best-practices-overview.html). But this is a very robust foundation to iterate on.

I wonder if we should put this in the tutorials section of the CockroachCloud docs? Also, what about a 20.1 version?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ericharmeling, @jseldess, and @rmloveland)

---

v19.2/multi-region-application.md, line 7 at r2 (raw file):

---

This page walks you through developing a multi-region application. It is the fourth section of the [Develop and Deploy a Multi-Region Web Application](multi-region-overview.html) guide.

nit: I'd prefer to refer to this as a "tutorial" rather than a "guide". You willing to make that change throughout?

---

v19.2/multi-region-application.md, line 11 at r2 (raw file):

## Before you begin

Before you begin this section, complete the previous section of the guide, [Set Up a Virtual Environment for Developing Multi-Region Applications](multi-region-setup.html). After you set up the database and development environment, you should be ready to look at the example application source code.

previous section of the guide > previous section of the tutorial

---

v19.2/multi-region-application.md, line 101 at r2 (raw file):

The first data structure that the file imports is `declarative_base`, a constructor for the [declarative base class](https://docs.sqlalchemy.org/en/13/orm/extensions/declarative/api.html#sqlalchemy.ext.declarative.declarative_base), built on SQLAlchemy's Declarative extension. All mapped objects inherit from this object. We assign a declarative base object to the `Base` variable right below the imports.

The `models.py` file also imports some other standard SQLAlchemy data structures that represent database objects (like columns), data types, and constraints, in addition to a standard Python library to help with default values (i.e. `datetime`).

i.e. > i.e.,

---

v19.2/multi-region-application.md, line 202 at r2 (raw file):

`run_transaction()` has the following additional benefits:

- When passed a [sqlalchemy.orm.session.sessionmaker](https://docs.sqlalchemy.org/en/latest/orm/session_api.html#session-and-sessionmaker) object, it ensures that a new session is created exclusively for use by the callback, which protects you from accidentally reusing objects via any sessions created outside the transaction. Note that a `sessionmaker` objects is different from a `session` object, which is not an allowable `transactor` for `run_transaction()`.

Should this be surrounded by backticks?

---

v19.2/multi-region-database.md, line 11 at r2 (raw file):

## Before you begin

Before you begin this section, complete the previous section of the guide, [MovR: An Example Multi-Region Use-Case](multi-region-use-case.html). After you are familiar with the fictional vehicle-sharing company MovR, and the challenges that global companies can solve by building multi-region applications, you should be ready to create a database schema for the application's database.

I'd remove the second sentence.

---

v19.2/multi-region-database.md, line 49 at r2 (raw file):

~~~ sql
PARTITION BY LIST (city) (
             |     PARTITION us_west VALUES IN (('seattle'), ('san francisco'), ('los angeles')),

Do we need the pipes? Also, add > prompt.

---

v19.2/multi-region-database.md, line 59 at r2 (raw file):

~~~ sql
ALTER PARTITION europe_west OF INDEX movr.public.users@primary CONFIGURE ZONE USING
             |     constraints = '[+region=gcp-europe-west1]';

Do we need the pipes? Also, add > prompt.

---

v19.2/multi-region-database.md, line 75 at r2 (raw file):

~~~
CREATE TABLE users (

Add > prompt and sql syntax highlighting.

---

v19.2/multi-region-database.md, line 101 at r2 (raw file):

~~~
CREATE TABLE vehicles (

Add > prompt and sql syntax highlighting.

---

v19.2/multi-region-database.md, line 126 at r2 (raw file):

- Like the `users` table, the `vehicles` table has a composite primary key on `city` and `id`.
- The `vehicles` table has a [foreign key constraint](foreign-key.html) on the `users` table, for the `city` and `owner_id` columns. This guarantees that a vehicle is registered to a particular user (i.e. an "owner") in the city where that user is registered.

i.e. > i.e.,

---

v19.2/multi-region-database.md, line 133 at r2 (raw file):

    ~~~ sql
    ALTER PARTITION us_east OF INDEX movr.public.vehicles@vehicles_auto_index_fk_city_ref_users CONFIGURE ZONE USING

Add > prompt.

---

v19.2/multi-region-database.md, line 144 at r2 (raw file):

~~~
  CREATE TABLE rides (

Add > prompt and sql syntax highlighting.

---

v19.2/multi-region-database.md, line 183 at r2 (raw file):

## Next steps

Now that you are familiar with the `movr` schema, you should be ready to [set up a development environment for a multi-region application](multi-region-setup.html).

I'd cut you should be ready to.

---

v19.2/multi-region-deployment.md, line 80 at r2 (raw file):

1. **Optional:** Enable the [Google Maps Embed API](https://console.cloud.google.com/apis/library), create an API key, restrict the API key to all subdomains of your domain name (e.g. `https://site.com/*`), and retrieve the API key.

{{site.data.alerts.callout_info}}

I think this callout needs to be indented; it's breaking the flow of the numbered list and causing the next number to start a 1.

---

v19.2/multi-region-deployment.md, line 120 at r2 (raw file):

    {% include copy-clipboard.html %}
    ~~~ shell
    $ gcloud config set compute/zone us-east1-b && \

We need to put each command in a distinct code block; otherwise, weird things happen in the rendered doc.

---

v19.2/multi-region-deployment.md, line 132 at r2 (raw file):

    {% include copy-clipboard.html %}
    ~~~ shell
    $ KUBECONFIG=~/mcikubeconfig gcloud container clusters get-credentials --zone=us-east1-b movr-us-east

We need to put each command in a distinct code block; otherwise, weird things happen in the rendered doc.

---

v19.2/multi-region-deployment.md, line 141 at r2 (raw file):

    {% include copy-clipboard.html %}
    ~~~ shell
    $ kubectl config use-context <context-name> && \

This one's fine because we're not using multiple prompts.

---

v19.2/multi-region-overview.md, line 17 at r2 (raw file):

1. [Deploy a Multi-Region Web Application](multi-region-deployment.html)

Throughout the guide, we reference the source code for an example web application for the fictional vehicle-sharing company [MovR](movr.html). The source code for this application is open source and available on GitHub, in the [movr-flask repo](https://github.com/cockroachlabs/movr-flask). The code is well-commented, with docstrings defined at the beginning of each class and function definition. The repo's [README](https://github.com/cockroachlabs/movr-flask/README.md) also includes instructions on debugging and deploying the application using Google Cloud services. Those instructions are reproduced in [Setting Up a Virtual Environment for Developing Multi-Region Applications](multi-region-setup.html) and [Deploying a Multi-Region Web Application](multi-region-deployment.html).

That link doesn't work for me. Perhaps use https://github.com/cockroachlabs/movr-flask/blob/master/README.md?

---

v19.2/multi-region-setup.md, line 11 at r2 (raw file):

## Before you begin

Before you begin this section, complete the previous section of the guide, [Create a Multi-Region Database Schema](multi-region-database.html). After you create a database schema for the application, you should be ready to set up an environment for developing and debugging.

I'd remove the second sentence.

Also, might be nice to present these prereqs as steps or bullets.

---

v19.2/multi-region-setup.md, line 50 at r2 (raw file):

    {% include copy-clipboard.html %}
    ~~~ shell
    $ cockroach sql --insecure --url='postgresql://root@127.0.0.1:62268/movr' < dbinit.sql

Might want to break into multiple lines.

---

v19.2/multi-region-use-case.md, line 15 at r2 (raw file):

## Resilience and distributed deployments

For an application to be resilient to system failures, the application server and database need to be deployed on multiple machines (i.e. part of a distributed deployment). In distributed CockroachDB deployments, all data is replicated and distributed across the instances of the database that make up the deployment.  For more information about data replication and distribution in CockroachDB, see [Life of Distributed Transaction](architecture/life-of-a-distributed-transaction.html).

i.e. > i.e.,

---

v19.2/multi-region-use-case.md, line 15 at r2 (raw file):

## Resilience and distributed deployments

For an application to be resilient to system failures, the application server and database need to be deployed on multiple machines (i.e. part of a distributed deployment). In distributed CockroachDB deployments, all data is replicated and distributed across the instances of the database that make up the deployment.  For more information about data replication and distribution in CockroachDB, see [Life of Distributed Transaction](architecture/life-of-a-distributed-transaction.html).

We may wan to link to one of our training videos on Youtube instead. Perhaps https://youtu.be/LgbrmIjH0cU or https://youtu.be/k5BR9m8o9ec?

---

v19.2/multi-region-use-case.md, line 17 at r2 (raw file):
I might cut the next paragraph and replace the last sentence of this second paragraph with something like:

This tutorial

---

v19.2/multi-region-use-case.md, line 36 at r2 (raw file):
I think part of this paragraph can be condensed a bit:

Geo-partitioning enables you to control where specific rows of data are stored, reducing the distance requests need to travel between the application and the database. In Creating a Multi-Region Database Schema, we walk you through geo-partitioning the example database.

---

v19.2/multi-region-use-case.md, line 39 at r2 (raw file):

{{site.data.alerts.callout_info}}
Geo-partitioned replicas can dramatically improve latency in multi-region deployments, but at the cost of resilience. Geo-partitioned replicas are resilient to availability zone failures, but not regional failures. When you geo-partition data, that data is no longer replicated and distributed in regions outside of a specific replication zone. In the event of a regional failure, the data in a partition becomes unavailable.

I'd cut everything from "When you geo-partition data..."

I might make "cost of resiliency" a link to https://www.cockroachlabs.com/docs/stable/topology-geo-partitioned-replicas.html#resiliency

---

v19.2/tutorials/multi-region-application.md, line 7 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

Great idea. I'm going to lead with a general description of the page, and then mention that it is part of a larger guide. After that, I'll make a "Before you begin" section and add reading the previous section to the list of requirements.

Not sure you need the second sentence.

[ericharmeling]

@jseldess Thanks for the review!

Fair point about covering more best practices. It will be difficult to cover all (or even most) in a single tutorial, but I'll open up an issue to explore adding other "Best Practice" guidance to this tutorial, and to the MovR app as a whole.

I'm in favor of adding this doc to the tutorials section of the CockroachCloud docs, but that shouldn't be its main and only home. The tutorial is not about CockroachCloud, but about building multi-region apps and should be placed in the "Developer Guide" or somewhere with higher visibility to developers.

As far as a 20.1 version is concerned, I'd need to deploy the app on top of a manual 20.1 multi-region deployment. At present, CC is only running 19.2. I can make an issue for that as well.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jseldess, and @rmloveland)

---

v19.2/multi-region-application.md, line 7 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

nit: I'd prefer to refer to this as a "tutorial" rather than a "guide". You willing to make that change throughout?

Done.

---

v19.2/multi-region-application.md, line 11 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

previous section of the guide > previous section of the tutorial

Done.

---

v19.2/multi-region-application.md, line 101 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

i.e. > i.e.,

Done.

---

v19.2/multi-region-application.md, line 202 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Should this be surrounded by backticks?

Added some.

---

v19.2/multi-region-database.md, line 11 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

I'd remove the second sentence.

Done.

---

v19.2/multi-region-database.md, line 49 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Do we need the pipes? Also, add > prompt.

Done.

---

v19.2/multi-region-database.md, line 59 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Do we need the pipes? Also, add > prompt.

Done.

---

v19.2/multi-region-database.md, line 75 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Add > prompt and sql syntax highlighting.

Done.

---

v19.2/multi-region-database.md, line 101 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Add > prompt and sql syntax highlighting.

Done.

---

v19.2/multi-region-database.md, line 126 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

i.e. > i.e.,

Done.

---

v19.2/multi-region-database.md, line 133 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Add > prompt.

Done.

---

v19.2/multi-region-database.md, line 144 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Add > prompt and sql syntax highlighting.

Done.

---

v19.2/multi-region-database.md, line 183 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

I'd cut you should be ready to.

Done.

---

v19.2/multi-region-deployment.md, line 80 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

I think this callout needs to be indented; it's breaking the flow of the numbered list and causing the next number to start a 1.

Done.

---

v19.2/multi-region-deployment.md, line 120 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

We need to put each command in a distinct code block; otherwise, weird things happen in the rendered doc.

Done.

---

v19.2/multi-region-deployment.md, line 132 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

We need to put each command in a distinct code block; otherwise, weird things happen in the rendered doc.

Done.

---

v19.2/multi-region-overview.md, line 17 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

That link doesn't work for me. Perhaps use https://github.com/cockroachlabs/movr-flask/blob/master/README.md?

Done.

---

v19.2/multi-region-setup.md, line 11 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

I'd remove the second sentence.

Also, might be nice to present these prereqs as steps or bullets.

Done.

---

v19.2/multi-region-setup.md, line 50 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Might want to break into multiple lines.

Done.

---

v19.2/multi-region-use-case.md, line 15 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

i.e. > i.e.,

Done.

---

v19.2/multi-region-use-case.md, line 15 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

We may wan to link to one of our training videos on Youtube instead. Perhaps https://youtu.be/LgbrmIjH0cU or https://youtu.be/k5BR9m8o9ec?

Done.

---

v19.2/multi-region-use-case.md, line 17 at r2 (raw file):
I cut the next paragraph.

I'm a little confused by what you mean by

and replace the last sentence of this second paragraph with something like:

This tutorial

---

v19.2/multi-region-use-case.md, line 36 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

I think part of this paragraph can be condensed a bit:

Geo-partitioning enables you to control where specific rows of data are stored, reducing the distance requests need to travel between the application and the database. In Creating a Multi-Region Database Schema, we walk you through geo-partitioning the example database.

Done.

---

v19.2/multi-region-use-case.md, line 39 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

I'd cut everything from "When you geo-partition data..."

I might make "cost of resiliency" a link to https://www.cockroachlabs.com/docs/stable/topology-geo-partitioned-replicas.html#resiliency

Done.

---

v19.2/tutorials/multi-region-application.md, line 7 at r1 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Not sure you need the second sentence.

Done.

---

v19.2/tutorials/multi-region-application.md, line 180 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Fair! That's a lot of code to try to wedge in here, as you say.

Done.

[cockroach-teamcity]

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/cd2f0f55264fc0034eadb43adbe5d7aba2c83927/

Edited pages:

• v19.2/multi-region-application.md
• v19.2/multi-region-database.md
• v19.2/multi-region-deployment.md
• v19.2/multi-region-overview.md
• v19.2/multi-region-setup.md
• v19.2/multi-region-use-case.md