Adar Dembo has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 25: (5 comments) http://gerrit.cloudera.org:8080/#/c/3952/25/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: PS25, Line 187: Microseconds > The idea was to monitor those values as closely as possible. Without resor Just to be clear, I meant "coarser" where I wrote "higher" granularity. That is, perhaps this could be as large of a value as 10ms, for example. It just struck me as a very tight loop. Tight loops are useful when trying to exercise a data race, but in this case, I would expect a full end-to-end Flush() to take on the order of 1ms, especially in a debug or slower build. If that's true, checking every 10us is excessive. But, it shouldn't slow down the tests significantly. If I'm the only one who feels this way, I'm fine with you leaving it as is. PS25, Line 2186: // Applying a big operation which does not fit into the buffer should : // return an error with session running in any supported flush mode. > Nope. This is different: this test verifies that an attempt to apply a _si Can you help me see the difference? AFAICT the second test in TestApplyTooMuchWithoutFlushing: 1. Starts a new session in MANUAL_FLUSH mode. 2. Customizes the buffer size. 3. Applies a single insert, sized to twice the buffer size. 4. Asserts that this fails with Incomplete. TestCheckMutationBufferSpaceLimitInEffect does the same thing across all flush modes, except that the buffer size is a little different, and that the insert isn't twice the buffer size, but only one byte larger. PS25, Line 2258: WaitForAutoFlushBackground(session, FLUSH_INTERVAL_MS); : // There should be some pending ops: the automatic background flush : // should not be active after switch to the MANUAL_FLUSH mode. > About 500 ms. Is that too much? Nah, I think that's fine. Anything on the order of multiple seconds is something we should think about. Line 2428: // AUTO_FLUSH_BACKGROUND should be faster than AUTO_FLUSH_SYNC. > Due to the nature of the things and chosen parameters, the difference is in Yes, and please try TSAN. It slows things down in unexpected ways (e.g. context switches become much more expensive, as does thread startup). PS25, Line 2574: ASSERT_TRUE(session->HasPendingOperations()); > Yes, I also have this concern. The principal solution would be delaying/st That's fine, just loop it using dist-test.py in TSAN mode, and I'll be satisfied. -- To view, visit http://gerrit.cloudera.org:8080/3952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8 Gerrit-PatchSet: 25 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
