Adar Dembo has posted comments on this change. Change subject: Allow AUTO_FLUSH_SYNC mode in C++ client ......................................................................
Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/2404/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: Line 1426: // Try inserting without specifying a key: should fail. > Whether in manual or auto flush mode, if key is not specified, apply should OK, this is an artifact of TestInsertSingleRowManualBatch. I thought that by including this code here, you were explicitly testing some key check behavior that exists with AUTO_FLUSH_SYNC but not with MANUAL_FLUSH. If the goal is to test the AUTO_FLUSH_SYNC works as expected, shouldn't there be a scan involved? Otherwise how would you know if Apply() triggered a Flush()? Or is that why you call session->HasPendingOperations() below? Anyway, I think this test is longer than it needs to be, probably due to the use of TestInsertSingleRowManualBatch as a template. Would you mind trimming it so that it tests exactly what should be tested for AUTO_FLUSH_SYNC? Line 1439: KuduWriteOperation* failed_op = error->release_failed_op(); > failed_op == ptr, and ptr is put into insert and reused again, those code a I see; because of the different types, I didn't expect ptr and failed_op to be the same. -- To view, visit http://gerrit.cloudera.org:8080/2404 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I43e9da85d2df5d350e35b41181db28685541eccd Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Binglin Chang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Binglin Chang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
