You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2021/05/13 05:06:51 UTC

[kudu-CR] KUDU-2612 automatically flush sessions on txn commit

Hello Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/17431

to look at the new patch set (#2).

Change subject: KUDU-2612 automatically flush sessions on txn commit
......................................................................

KUDU-2612 automatically flush sessions on txn commit

With this patch, all transactional sessions created off a transction
handle are automatically flushed upon calling Commit() on the handle.

As for the KuduTransaction::StartCommit() method, it's now necessary
to flush all the transactional sessions created off the transaction
handle before calling the method, otherwise Status::IllegalState()
would be returned.

As Andrew and I discussed offline, it might be an option to return an
error from KuduSession::Apply() for a transactional session whose
transaction has already started committing.  However, after looking at
this closer, I realized that it would a bit tricky given it should be
thread-safe, so it would add more complexity into the hot path of
applying write operations in the context of a session.  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.

Another design detail worth pointing at is that a KuduTransaction handle
keeps shared, not weak pointers to transaction sessions originated off
the handle (I did several back-and-forth iterations on this, though).
Even if using shared_ptr, not weak_ptr, no circular dependencies are
introduced since a transactional session doesn't keep a reference
to the corresponding transactional handle.  The shared_ptr-based
approach looks better than one with weak_ptr because
  (1) It might prevent a data loss due to a mistake in an application
      code, and it takes time to find and fix those.
  (2) It looks more portable and consistent if thinking about similar
      functionality to implement in the Java client.

This patch also contains several test scenarios for the newly introduced
functionality.

Change-Id: I2480129a99fb19d16868e14f9b9e33c83e3d8e7f
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/transaction-internal.cc
M src/kudu/client/transaction-internal.h
M src/kudu/integration-tests/txn_write_ops-itest.cc
5 files changed, 670 insertions(+), 59 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/17431/2
-- 
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: newpatchset
Gerrit-Change-Id: I2480129a99fb19d16868e14f9b9e33c83e3d8e7f
Gerrit-Change-Number: 17431
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)