From c3c2435228ab7dee6f84a7fd4549b6c6e5884dc8 Mon Sep 17 00:00:00 2001
From: sfc-gh-tclinkenbeard <trevor.clinkenbeard@snowflake.com>
Date: Fri, 28 Oct 2022 20:48:18 -0700
Subject: [PATCH] Addressed review comments

---
 fdbserver/GlobalTagThrottler.actor.cpp | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fdbserver/GlobalTagThrottler.actor.cpp b/fdbserver/GlobalTagThrottler.actor.cpp
index ac69d06806..5ddea3d618 100644
--- a/fdbserver/GlobalTagThrottler.actor.cpp
+++ b/fdbserver/GlobalTagThrottler.actor.cpp
@@ -311,8 +311,8 @@ class GlobalTagThrottlerImpl {
 	// cost and throttling ratio
 	Optional<double> getLimitingCost(UID storageServerId) const {
 		auto const ssInfo = tryGet(ssInfos, storageServerId);
-		auto const throttlingRatio = ssInfo.present() ? ssInfo.get().throttlingRatio : Optional<double>{};
-		auto const currentCost = getCurrentCost(storageServerId);
+		Optional<double> const throttlingRatio = ssInfo.present() ? ssInfo.get().throttlingRatio : Optional<double>{};
+		Optional<double> const currentCost = getCurrentCost(storageServerId);
 		if (!throttlingRatio.present() || !currentCost.present()) {
 			return {};
 		}
@@ -322,8 +322,8 @@ class GlobalTagThrottlerImpl {
 	// For a given storage server and tag combination, return the limiting transaction rate.
 	Optional<double> getLimitingTps(UID storageServerId, TransactionTag tag) const {
 		auto const quotaRatio = getQuotaRatio(tag, storageServerId);
-		auto const limitingCost = getLimitingCost(storageServerId);
-		auto const averageTransactionCost = getAverageTransactionCost(tag, storageServerId);
+		Optional<double> const limitingCost = getLimitingCost(storageServerId);
+		Optional<double> const averageTransactionCost = getAverageTransactionCost(tag, storageServerId);
 		if (!limitingCost.present() || !averageTransactionCost.present()) {
 			return {};
 		}
@@ -334,10 +334,12 @@ class GlobalTagThrottlerImpl {
 
 	// Return the limiting transaction rate, aggregated across all storage servers.
 	// The limits from the worst maxFallingBehind zones are
-	// ignored
+	// ignored, because we do not non-workload related issues (e.g. slow disks)
+	// to affect tag throttling. If more than maxFallingBehind zones are at
+	// or near saturation, this indicates that throttling should take place.
 	Optional<double> getLimitingTps(TransactionTag tag) const {
 		// TODO: The algorithm for ignoring the worst zones can be made more efficient
-		std::map<Optional<Standalone<StringRef>>, double> zoneIdToLimitingTps;
+		std::unordered_map<Optional<Standalone<StringRef>>, double> zoneIdToLimitingTps;
 		for (const auto& [id, ssInfo] : ssInfos) {
 			auto const limitingTpsForSS = getLimitingTps(id, tag);
 			if (limitingTpsForSS.present()) {
@@ -749,6 +751,8 @@ Future<Void> monitorActor(GlobalTagThrottler* globalTagThrottler, Check check) {
 	loop {
 		wait(delay(1.0));
 		if (check(*globalTagThrottler)) {
+			// Wait for 10 consecutive successes so we're certain
+			// than a stable equilibrium has been reached
 			if (++successes == 10) {
 				return Void();
 			}