Adar Dembo has posted comments on this change.
Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
Patch Set 25:
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
: // return an error with session running in any supported flush
> 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
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
: // should not be active after switch to the MANUAL_FLUSH
> 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
To view, visit http://gerrit.cloudera.org:8080/3952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>