Todd Lipcon has posted comments on this change. Change subject: Add per tablet write RPC throttling ......................................................................
Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/2327/3/src/kudu/integration-tests/write_throttling-itest.cc File src/kudu/integration-tests/write_throttling-itest.cc: Line 51: class WriteThrottlingTest : public KuduTest { if you extend ExternalMiniClusterITestBase you get most of the code below for free to start/stop cluster Line 112: session->SetTimeoutMillis(1000); afraid this might be a bit short and cause it to be flaky Line 126: LOG(INFO) << "Batch " << t << " qps: " << qps; I think "iteration" is better than "batch" here, because you aren't actually batching (since you use AUTO_FLUSH_SYNC) Line 158: LOG(INFO) << "Batch " << t << " qps: " << qps << " " << bps << " byte/s"; same http://gerrit.cloudera.org:8080/#/c/2327/3/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: Line 376: bool Throttle(int64_t bytes); can you rename this to 'ShouldThrottleAllow' so that it's clearer whether 'true' means 'reject this RPC' or 'allow this RPC'? Line 528: gscoped_ptr<Throttler> throttler_; can you use std::unique_ptr here now that we are C++11? (should start using that from now on, even though we haven't updated all old code yet) http://gerrit.cloudera.org:8080/#/c/2327/3/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 686: // TODO: include sidecar bytes. hm, I dont think this TODO is relevant since we don't currently use the sidecar on the writer side. Line 687: if (!tablet->Throttle(req->ByteSize())) { - is it more appropriate to use row_operations.rows.size() + row_operations.indirect_data.size()? Otherwise users who have large schemas might be penalized unfairly (plus this will use less CPU vs the ByteSize call) - also, do you think we should throttle based on the number of rows written instead of number of RPCs? Line 690: TabletServerErrorPB::UNKNOWN_ERROR, let's introduce a new enum like WRITE_THROTTLED here http://gerrit.cloudera.org:8080/#/c/2327/3/src/kudu/util/throttler.h File src/kudu/util/throttler.h: Line 41: explicit Throttler(MonoTime now, uint64_t op_rate, uint64_t byte_rate, double burst_factor) : - dont need 'explicit' on multi-arg constructor - can you clarify in the docs (and maybe the names) that the rates are per second (rather than per refill period)? Line 51: bool Take(MonoTime now, uint64_t op, uint64_t byte) { I think these methods are long/complex enough they should be in a throttler.cc Line 76: uint64_t num_period = d / kRefillPeriodMicros + 1; am wondering whether it would make more sense to make this a double (and not + 1), and then always set next_refill_ = now + kRefillPeriodMicros? It seems like that might be 'smoother' than using the integer math -- To view, visit http://gerrit.cloudera.org:8080/2327 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I33cb6934d27b883a783682cef1e0723100637d45 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Binglin Chang <[email protected]> Gerrit-Reviewer: Binglin Chang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
