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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 02 Jun 2021 01:09:11 +0000 Gerrit-HasComments: Yes
