Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17431 )
Change subject: KUDU-2612 automatically flush sessions on txn commit ...................................................................... Patch Set 2: (3 comments) Thank you for the feedback! http://gerrit.cloudera.org:8080/#/c/17431/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17431/2//COMMIT_MSG@22 PS2, Line 22: So, I opted : not to perform the consistency check as a part of KuduSession::Apply(), : rather relying on the logic of KuduSession::Flush() and : KuduSession::FlushAsync() methods instead. > This is true even for Commit() (synchronous), right? Once calling commit, u Right -- it doesn't matter whether it's StartCommit() or Commit(), the point is that any concurrent KuduSession.Apply() might bet write operations left aside. Yes, in other than AUTO_FLUSH_SYNC modes users of the API can still call KuduSession.Apply() after commit and not get error, and the error will be reported only upon KuduSession.Flush() called implicitly or explicitly. As I mentioned, we can address that by adding an atomic boolean into a session (and that was so at some point in one of local revisions of this patch), but I'm not sure we want to protect from that. I think our effort here is to protect against programmatic errors, rather than from very intricate made-up use cases. http://gerrit.cloudera.org:8080/#/c/17431/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/17431/2/src/kudu/client/client-test.cc@7433 PS2, Line 7433: ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH)); > I do find it a bit odd for us to be implicitly flushing on Commit() even if That looks like a viable option, but why would anybody add rows into a buffer via KuduSession.Apply() just to have those rows dropped on the floor later if they don't call KuduSession.Flush() before calling KuduTransaction.Commit()? I guess in that sense KuduTransction.Commit() implies flushing all existing transaction session, and we just call Flush() to make life easier for the users of Kudu client API. It would be great to do so also for StartCommit(), but the implementation has many more quirks there, that's why we opted just returning an error to warn people about possible data loss as early as possible. Also, since the flush mode can be dynamically updated, that makes it a bit trickier to judge when to sample the mode of the session for the sort of verification that you mentioned. I.e., what if the mode for the session was AUTO_FLUSH_BACKGROUND, and just before the commit they change it to MANUAL_FLUSH, but didn't add any new rows? In other words, I think doing such a differentiation based on the flush mode would add more quirks and lessen the ease of use of the API. Personally, I'd also change the behavior of KuduSession to automatically flush all buffered operations on destruction, but that's another story. I guess I can add a note that auto-flushing behavior is there just for convenience and to avoid unintentionally dropping the data on the floor. What do you think? http://gerrit.cloudera.org:8080/#/c/17431/2/src/kudu/client/transaction-internal.cc File src/kudu/client/transaction-internal.cc: http://gerrit.cloudera.org:8080/#/c/17431/2/src/kudu/client/transaction-internal.cc@198 PS2, Line 198: // It's a rare case, but nothing prevents to call session.reset() or : // session.swap(nullptr) for a transactional session in the user code. > Doesn't this just replace the pointer in the scoped_refptr instance with a Good point -- that has become a BS when session is std::shared_ptr. Sure, the operations in the user code doesn't affect the copy stored in the txn_sessions_. -- To view, visit http://gerrit.cloudera.org:8080/17431 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2480129a99fb19d16868e14f9b9e33c83e3d8e7f Gerrit-Change-Number: 17431 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 14 May 2021 03:02:52 +0000 Gerrit-HasComments: Yes
