Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 )
Change subject: KUDU-2612 tablet servers automatically register txn participants ...................................................................... Patch Set 12: (9 comments) http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc File src/kudu/integration-tests/txn_write_ops-itest.cc: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@227 PS12, Line 227: quals > nit: incomplete? Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@648 PS12, Line 648: CHECK(d); : return d; > nit: could combine as return DCHECK_NOTNULL(d) Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@1558 PS12, Line 1558: DISABLED_NonTxnWrites > Seems this was merged as https://gerrit.cloudera.org/c/17120/? One small funny nit is that this scenario is a bit different :) The essence is at line 1595: ASSERT_OK(CountRows(table_.get(), &row_count)); Here, some other table handle is used to read the data: 'table_', but the data was written using 'table' handle. This scenario still fails time to time, and if running in READ_YOUR_WRITES mode it fails more often (like 4 out of 256 vs 70 out of 256): 1596: Failure Expected equality of these values: 8 row_count Which is: 7 I was thinking to keep this around to clarify on this later, but I agree it's not the best place to keep it anyways, because it's not related to transactional writes or writes which are somehow affected by the newly introduced functionality. Probably, I'll need to open a separate review item for that, and corresponding upstream JIRA for that. Removed. http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h@416 PS12, Line 416: write specified > nit: specified write operation? Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h@450 PS12, Line 450: respond > nit: drop Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h@460 PS12, Line 460: are > nit: dropped off? Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc@1147 PS12, Line 1147: TxnBegin > nit: BeginTxn here too? Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc@1209 PS12, Line 1209: WARN_NOT_OK(self->Submit(), "fail to submit pending txn write requests"); > What if this fails with a transient error? Seems like we may fail to submit Good catch! Indeed, it's necessary to call UnregisterTxnDispatcher in that case. Done. http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc@1226 PS12, Line 1226: DCHECK(!preliminary_tasks_completed_); > Should be done under lock? Done. It will be cleaner this way, right. But it's not strictly necessary since this is the only method changing the 'preliminary_tasks_completed_' field and it's to be called only once. -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 12 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 02 Mar 2021 21:24:55 +0000 Gerrit-HasComments: Yes
