Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16194 )
Change subject: KUDU-2612 p6: add coordination calls to system client ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/integration-tests/auth_token_expire-itest.cc File src/kudu/integration-tests/auth_token_expire-itest.cc: http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/integration-tests/auth_token_expire-itest.cc@470 PS6, Line 470: ASSERT_OK(txn_client->OpenTxnStatusTable()); > If thinking about resetting all connection and re-acquiring the authn token Wouldn't the connection already have been reset by the cluster restart? The idea here was to acquire an initial authn token, wait for it to expire, and then use the expired token to begin the transaction -- I'll update the comment. http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/integration-tests/txn_status_table-itest.cc File src/kudu/integration-tests/txn_status_table-itest.cc: http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/integration-tests/txn_status_table-itest.cc@426 PS6, Line 426: vector<thread> threads; : Status statuses[2 * kPartitionWidth]; : for (int t = 0; t < 2; t++) { : threads.emplace_back([&, t] { > Would this need some update once transaction ID ordering becomes important That isn't a constraint server-side yet if inserting to different partitions of the transaction status table. In most cases I expect there will likely be only one active range at a time, which lessens the priority of absolute monotonic increasing ordering. http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/integration-tests/txn_status_table-itest.cc@430 PS6, Line 430: NOTE: we > Does this work as expected when the underlying operation returns non-OK sta Done http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/integration-tests/txn_status_table-itest.cc@532 PS6, Line 532: > drop Done http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/tablet/txn_coordinator.h File src/kudu/tablet/txn_coordinator.h: http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/tablet/txn_coordinator.h@46 PS6, Line 46: Starts a transaction with the given ID as the given user > Here and below: does it worth documenting the 'ts_error' output parameter? Done http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/transactions/txn_system_client.cc File src/kudu/transactions/txn_system_client.cc: http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/transactions/txn_system_client.cc@177 PS6, Line 177: First, take ownership of the context. > Why not to use unique_ptr<TxnStatusTabletContext> ctx_x(ctx.release()) here That runs the risk of having 'ctx' going out of scope and being destructed before this lambda gets run. http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/16194/6/src/kudu/tserver/tablet_service.cc@1231 PS6, Line 1231: need be > nit: needed ? Both are grammatically correct. -- To view, visit http://gerrit.cloudera.org:8080/16194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4126cb3dcf379b397f84578c2265dca3ece3d98c Gerrit-Change-Number: 16194 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 31 Jul 2020 21:52:10 +0000 Gerrit-HasComments: Yes
