Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16343 )
Change subject: KUDU-1587 part 2: reject write ops if apply queue is overloaded ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/16343/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16343/6//COMMIT_MSG@23 PS6, Line 23: new behavior is not yet enabled by default > What is the rational for not enabling by default? Is there some threshold_m I think we can put 1000 ms (i.e. one second) as somewhat safe default value. However, due to the timing of the upcoming release, I don't think I have enough time to make exhaustive testing of various workloads to be sure the default value is completely safe. But I'll try to run various workloads, at least those that Andrew pointed at. http://gerrit.cloudera.org:8080/#/c/16343/6//COMMIT_MSG@23 PS6, Line 23: This new behavior is not yet enabled by default, keeping the legacy : behavior of unbounded/uncontrolled queue times as is. To enable it, : set --tablet_apply_pool_overload_threshold_ms to something greater : than 0 (e.g., 500). > If not in an automated test, it'd be great if you could manually run a simi I added OpApplyQueueOverloadedTest.ClientRetriesOperations (based on SameTabletConcurrentWritesTest). The new test scenario verifies that the throttled requests are retried. This new test scenario shows almost the same request rate with and without op apply queue-based throttling: at my test machine, it's 492.25 vs 487 req/sec with 100 ms delay injected into the apply phase of a write operation. I'll take a look at how it looks if running against a real cluster with real workload. http://gerrit.cloudera.org:8080/#/c/16343/6/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/16343/6/src/kudu/tablet/tablet_replica.h@411 PS6, Line 411: Random rng_; > Shouldn't we be using ThreadSafeRandom, given we call this from the reactor Whoops. I thought Random was thread-safe itself. http://gerrit.cloudera.org:8080/#/c/16343/6/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/16343/6/src/kudu/tablet/tablet_replica.cc@456 PS6, Line 456: size_factor > I am not surely I fully understand why we want to give preference to smalle The intuition was that smaller batches put less load on the apply queue, so rejecting heavier patches would lead returning to non-overloaded queue faster. Another point was to give workloads with small batches higher priority, assuming that the load on the apply queue is induced mostly by workloads with larger batches. We discussed this offline. The resolution is that without backing this by actual evidence provided by tests, it's better to not discriminate based on the size of batches. An alternative might be keeping the size factor, but making it configurable via a flag, but it looks exactly as a YAGNI case. So, I'm removing the size factor for now: we can add it later on once we have more data on this. http://gerrit.cloudera.org:8080/#/c/16343/6/src/kudu/tablet/tablet_replica.cc@445 PS6, Line 445: // If the apply queue is overloaded, reject the incoming operation with : // some probability. : MonoDelta queue_otime; : MonoDelta threshold; : if (apply_pool_->QueueOverloaded(&queue_otime, &threshold)) { : auto overload_threshold_ms = threshold.ToMilliseconds(); : // The longer the queue has been in the overloaded state, the higher the : // probability of rejection. : auto time_factor = queue_otime.ToMilliseconds() / overload_threshold_ms + 1; : // The heavier the request in terms of number of rows, the higher the : // probability of rejecting it if the apply queue is overloaded. : auto size_factor = op_state->request()->row_operations().rows().size(); : auto factor = 1 + size_factor * time_factor; : if (!rng_.OneIn(factor)) { : return Status::ServiceUnavailable("op apply queue is overloaded"); : } : } > +1 to this if it's not to complicated. Done http://gerrit.cloudera.org:8080/#/c/16343/6/src/kudu/tablet/tablet_replica.cc@445 PS6, Line 445: // If the apply queue is overloaded, reject the incoming operation with : // some probability. : MonoDelta queue_otime; : MonoDelta threshold; : if (apply_pool_->QueueOverloaded(&queue_otime, &threshold)) { : auto overload_threshold_ms = threshold.ToMilliseconds(); : // The longer the queue has been in the overloaded state, the higher the : // probability of rejection. : auto time_factor = queue_otime.ToMilliseconds() / overload_threshold_ms + 1; : // The heavier the request in terms of number of rows, the higher the : // probability of rejecting it if the apply queue is overloaded. : auto size_factor = op_state->request()->row_operations().rows().size(); : auto factor = 1 + size_factor * time_factor; : if (!rng_.OneIn(factor)) { : return Status::ServiceUnavailable("op apply queue is overloaded"); : } : } > Could we instead bake this into the TabletService endpoint? We already do s Done http://gerrit.cloudera.org:8080/#/c/16343/6/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/16343/6/src/kudu/tserver/tablet_server-test.cc@4375 PS6, Line 4375: intjected > nit: injected Done http://gerrit.cloudera.org:8080/#/c/16343/6/src/kudu/tserver/tablet_server-test.cc@4468 PS6, Line 4468: that > nit: rather than Done -- To view, visit http://gerrit.cloudera.org:8080/16343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d7688d6fa832e606b8efc4549568fa52dfa1931 Gerrit-Change-Number: 16343 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 26 Aug 2020 08:41:35 +0000 Gerrit-HasComments: Yes
