From 5a8adca1f7a7283a4f225f78a3c5b8143c80d535 Mon Sep 17 00:00:00 2001 From: Xiaoxi Wang Date: Tue, 25 Oct 2022 10:45:21 -0700 Subject: [PATCH] solve review comments: mark const; add comments; template abbreviation --- fdbclient/include/fdbclient/NativeAPI.actor.h | 3 ++- fdbserver/DataDistribution.actor.cpp | 14 ++++++++++++++ .../include/fdbserver/DataDistribution.actor.h | 14 +------------- fdbserver/include/fdbserver/MockGlobalState.h | 9 +++++---- .../fdbserver/ShardsAffectedByTeamFailure.h | 4 ++-- fdbserver/include/fdbserver/StorageMetrics.actor.h | 4 ++-- fdbserver/storageserver.actor.cpp | 10 +++++----- 7 files changed, 31 insertions(+), 27 deletions(-) diff --git a/fdbclient/include/fdbclient/NativeAPI.actor.h b/fdbclient/include/fdbclient/NativeAPI.actor.h index d1f4860f23..642a4e747a 100644 --- a/fdbclient/include/fdbclient/NativeAPI.actor.h +++ b/fdbclient/include/fdbclient/NativeAPI.actor.h @@ -602,7 +602,8 @@ ACTOR Future> waitStorageMetricsWithLocation(TenantInfo StorageMetrics permittedError); // Return the suggested split points from storage server.The locations tell which interface should -// serve the request. The +// serve the request. `limit` is the current estimated storage metrics of `keys`.The returned points, if present, +// guarantee the metrics of split result is within limit. ACTOR Future>>> splitStorageMetricsWithLocations( std::vector locations, KeyRange keys, diff --git a/fdbserver/DataDistribution.actor.cpp b/fdbserver/DataDistribution.actor.cpp index 9f01a1b2be..10b168b7b3 100644 --- a/fdbserver/DataDistribution.actor.cpp +++ b/fdbserver/DataDistribution.actor.cpp @@ -53,6 +53,20 @@ #include "fdbserver/DDSharedContext.h" #include "flow/actorcompiler.h" // This must be the last #include. +ShardSizeBounds ShardSizeBounds::shardSizeBoundsBeforeTrack() { + return ShardSizeBounds{ + .max = StorageMetrics{ .bytes = -1, + .bytesPerKSecond = StorageMetrics::infinity, + .iosPerKSecond = StorageMetrics::infinity, + .bytesReadPerKSecond = StorageMetrics::infinity }, + .min = StorageMetrics{ .bytes = -1, .bytesPerKSecond = 0, .iosPerKSecond = 0, .bytesReadPerKSecond = 0 }, + .permittedError = StorageMetrics{ .bytes = -1, + .bytesPerKSecond = StorageMetrics::infinity, + .iosPerKSecond = StorageMetrics::infinity, + .bytesReadPerKSecond = StorageMetrics::infinity } + }; +} + struct DDAudit { DDAudit(UID id, KeyRange range, AuditType type) : id(id), range(range), type(type), auditMap(AuditPhase::Invalid, allKeys.end), actors(true) {} diff --git a/fdbserver/include/fdbserver/DataDistribution.actor.h b/fdbserver/include/fdbserver/DataDistribution.actor.h index 2e77d07459..ff33386233 100644 --- a/fdbserver/include/fdbserver/DataDistribution.actor.h +++ b/fdbserver/include/fdbserver/DataDistribution.actor.h @@ -477,19 +477,7 @@ struct ShardSizeBounds { return max == rhs.max && min == rhs.min && permittedError == rhs.permittedError; } - static ShardSizeBounds shardSizeBoundsBeforeTrack() { - return ShardSizeBounds{ - .max = StorageMetrics{ .bytes = -1, - .bytesPerKSecond = StorageMetrics::infinity, - .iosPerKSecond = StorageMetrics::infinity, - .bytesReadPerKSecond = StorageMetrics::infinity }, - .min = StorageMetrics{ .bytes = -1, .bytesPerKSecond = 0, .iosPerKSecond = 0, .bytesReadPerKSecond = 0 }, - .permittedError = StorageMetrics{ .bytes = -1, - .bytesPerKSecond = StorageMetrics::infinity, - .iosPerKSecond = StorageMetrics::infinity, - .bytesReadPerKSecond = StorageMetrics::infinity } - }; - } + static ShardSizeBounds shardSizeBoundsBeforeTrack(); }; // Gets the permitted size and IO bounds for a shard diff --git a/fdbserver/include/fdbserver/MockGlobalState.h b/fdbserver/include/fdbserver/MockGlobalState.h index a404f24027..5f6109626d 100644 --- a/fdbserver/include/fdbserver/MockGlobalState.h +++ b/fdbserver/include/fdbserver/MockGlobalState.h @@ -113,11 +113,12 @@ public: void getStorageMetrics(const GetStorageMetricsRequest& req) override; template - using isLoadBalancedReply = std::is_base_of; + static constexpr bool isLoadBalancedReply = std::is_base_of_v; template - typename std::enable_if::value, void>::type - sendErrorWithPenalty(const ReplyPromise& promise, const Error& err, double penalty) { + typename std::enable_if_t, void> sendErrorWithPenalty(const ReplyPromise& promise, + const Error& err, + double penalty) { Reply reply; reply.error = err; reply.penalty = penalty; @@ -125,7 +126,7 @@ public: } template - typename std::enable_if::value, void>::type + typename std::enable_if_t, void> sendErrorWithPenalty(const ReplyPromise& promise, const Error& err, double) { promise.sendError(err); } diff --git a/fdbserver/include/fdbserver/ShardsAffectedByTeamFailure.h b/fdbserver/include/fdbserver/ShardsAffectedByTeamFailure.h index 9055098bc7..7b674510d4 100644 --- a/fdbserver/include/fdbserver/ShardsAffectedByTeamFailure.h +++ b/fdbserver/include/fdbserver/ShardsAffectedByTeamFailure.h @@ -80,8 +80,8 @@ public: bool hasShards(Team team) const; // The first element of the pair is either the source for non-moving shards or the destination team for in-flight - // shards The second element of the pair is all previous sources for in-flight shards. This function only return the - // teams for the first shard in [keys.begin, keys.end) + // shards. The second element of the pair is all previous sources for in-flight shards. This function only returns + // the teams for the first shard in [keys.begin, keys.end) std::pair, std::vector> getTeamsForFirstShard(KeyRangeRef keys); std::pair, std::vector> getTeamsFor(KeyRef key); diff --git a/fdbserver/include/fdbserver/StorageMetrics.actor.h b/fdbserver/include/fdbserver/StorageMetrics.actor.h index 552db2c6f7..dc518cf318 100644 --- a/fdbserver/include/fdbserver/StorageMetrics.actor.h +++ b/fdbserver/include/fdbserver/StorageMetrics.actor.h @@ -163,9 +163,9 @@ public: StorageServerMetrics metrics; // penalty used by loadBalance() to balance requests among service instances - virtual double getPenalty() { return 1; } + virtual double getPenalty() const { return 1; } - virtual bool isReadable(KeyRangeRef const& keys) { return true; } + virtual bool isReadable(KeyRangeRef const& keys) const { return true; } virtual void addActor(Future future) = 0; diff --git a/fdbserver/storageserver.actor.cpp b/fdbserver/storageserver.actor.cpp index 1d8fb40c8f..1472b4dcaa 100644 --- a/fdbserver/storageserver.actor.cpp +++ b/fdbserver/storageserver.actor.cpp @@ -807,8 +807,8 @@ public: VersionedData const& data() const { return versionedData; } VersionedData& mutableData() { return versionedData; } - double old_rate = 1.0; - double currentRate() { + mutable double old_rate = 1.0; + double currentRate() const { auto versionLag = version.get() - durableVersion.get(); double res; if (versionLag >= SERVER_KNOBS->STORAGE_DURABILITY_LAG_HARD_MAX) { @@ -1379,7 +1379,7 @@ public: // This is the maximum version that might be read from storage (the minimum version is durableVersion) Version storageVersion() const { return oldestVersion.get(); } - bool isReadable(KeyRangeRef const& keys) override { + bool isReadable(KeyRangeRef const& keys) const override { auto sh = shards.intersectingRanges(keys); for (auto i = sh.begin(); i != sh.end(); ++i) if (!i->value()->isReadable()) @@ -1405,10 +1405,10 @@ public: } } - Counter::Value queueSize() { return counters.bytesInput.getValue() - counters.bytesDurable.getValue(); } + Counter::Value queueSize() const { return counters.bytesInput.getValue() - counters.bytesDurable.getValue(); } // penalty used by loadBalance() to balance requests among SSes. We prefer SS with less write queue size. - double getPenalty() override { + double getPenalty() const override { return std::max(std::max(1.0, (queueSize() - (SERVER_KNOBS->TARGET_BYTES_PER_STORAGE_SERVER - 2.0 * SERVER_KNOBS->SPRING_BYTES_STORAGE_SERVER)) /