[kudu-CR] [docs] add a design overview for multi-row transactions

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17525 )

Change subject: [docs] add a design overview for multi-row transactions
......................................................................


Patch Set 2:

(23 comments)

Thank you for putting together this document!

Overall looks good to me, just nits so far.

http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc
File docs/design-docs/transactions.adoc:

http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@27
PS2, Line 27: To enable
Is it worth mentioning that at least 3 tablet servers are required in a Kudu cluster as well?  That's due to the default value for the --txn_manager_status_table_num_replicas flag.


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@33
PS2, Line 33: new terminology
Do we have a special term to denote a component that uses  txn-related API as part of a Kudu client?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@40
PS2, Line 40:  distributed table with a rough schema
Is this also some sort of 'logical' schema?  If so, does it make sense to mention that the timestamp of txn start is also stored as part of txn metadata?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@58
PS2, Line 58: Before delving into the details of each component
nit: is it worth mentioning that the txn status table is assumed to be of RF=3 or higher?  Otherwise, maybe use 'stored' instead of 'replicated' when referring to persisting a record in the txn status table?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@69
PS2, Line 69: Upon successfully Raft committing the status record
nit: what about

  Upon successfully persisting the new status record in the transaction status table


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@98
PS2, Line 98: the commit
... that the commit ... ?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@103
PS2, Line 103: a proxy
nit: as proxies ?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@132
PS2, Line 132: once the first BeginTransaction() request is received
Does it make sense to throw in an info about configuring this behavior using the --txn_manager_lazily_initialized flag?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@134
PS2, Line 134: ranges are added
Is it worth mentioning the --txn_manager_status_table_range_partition_span flag if it's necessary to configure range boundaries for a txn status table?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@178
PS2, Line 178: majority of
             : replicas
nit: the transaction status tablet's replicas

?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@191
PS2, Line 191: will be blocked
nit: will be blocked by the Raft protocol ?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@196
PS2, Line 196: Background orchestration tasks
An incomplete sentence or the paragraph marking is missing?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@219
PS2, Line 219: The `TxnStatusManagers` get heartbeat to by clients via the `TxnManager` in order to identify
             : abandoned transactions
What about:

Clients send heartbeat messages to a `TxnStatusManager` in order to let them know that a transaction isn't abandoned.  If a `TxnStatusManager' automatically aborts abandoned transactions.


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@225
PS2, Line 225: are aborted
nit: are automatically aborted


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@266
PS2, Line 266: map
nit: registry

I guess it's not very important what sort of container is used there, right?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@270
PS2, Line 270: temporarily and asynchronously uses the
             : `TxnSystemClient` to asynchronously register
maybe, replace with:

... temporarily, using the `TxnSystemClient` to register ...


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@273
PS2, Line 273: resubmitted
submitted


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@279
PS2, Line 279: removed from the map
nit: unregistered


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@351
PS2, Line 351: a transaction
nit: a multi-row transaction inserting rows into the same tablet

?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@356
PS2, Line 356: which serve
Does it make sense to mention that txn handles, while kept in scope, are also used to automatically send heartbeats to the backend, keeping a transaction from being automatically aborted due to staleness?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@367
PS2, Line 367: KUDU_RETURN_NOT_OK(session->Flush());
This piece isn't strictly necessary, but I guess we want to keep it here just to show proper discipline while working with sessions, right?


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@384
PS2, Line 384: Users
Client applications


http://gerrit.cloudera.org:8080/#/c/17525/2/docs/design-docs/transactions.adoc@414
PS2, Line 414: by passing `SerializationOptions`
nit: by passing customized `SerializationOptions`



-- 
To view, visit http://gerrit.cloudera.org:8080/17525
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14c5a8cbd2b239c68e355910e9a6de4576508dd6
Gerrit-Change-Number: 17525
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 02 Jun 2021 01:09:11 +0000
Gerrit-HasComments: Yes