Skip to content

Commit

Permalink
Command argument parsing improvements (#255)
Browse files Browse the repository at this point in the history 
* Add internal ArgSlice.FromPinnedSpan

* Simplify paths where WriteIntegerFromBytes is used

* We can avoid allocating intermediary arrays and extra fixing now

* Avoid ASCII re-encoding and string allocation in ReadDoubleWithLengthHeader

* Remove unused ReadPtrArrayWithLengthHeader

* Remove unused ReadArrayWithLengthHeader

* Avoid ASCII re-encoding and string allocation in more places

* Also few other nits for speed

* Use StringComparison.OrdinalIgnoreCase instead of ToUpper where appropiate

* Avoid string allocation & avoid current thread culture lookup for equals

* Switched some ToUpper to ToUpperInvariant too

* ToLower -> ToLowerInvariant

* Avoid CurrentCulture lookup

* Use Enum.TryParse instead of paying the cost exception

* Also switch over to generic overloads, allows JIT to see what type is used

* dotnet-format

* Revert BitmapCommand Parse overload changes

* We completely spanify these later, the calls got ugly

* Update BitmapCommands.cs

Missed one.

* dotnet-format

* Thanks github web editor

* Update libs/cluster/Session/ClusterCommands.cs

* FromFixedSpan -> FromPinnedSpan

* Add import lost in merge

* Apply suggestions from code review

* Remove regex from ACLParser

* Use string.Equals in ACLParser

* Make cluster node id comparisons use StringComparison.Ordinal

* instead of CurrentCulture

* Move more exact comparisons over to StringComparison.Ordinal

* Remove partial from ACLParser

* Leftover when I tested Regex source-gen but it's overkill here

* Fix floating point formatting for cultures with different decimal seperator

All tests finally pass for fi-FI

* Fix floating point formatting for cultures with different decimal seperator

* couple more

* Fix double.TryParse overload for .NET 6

* Use Enum.TryParse instead of exception handling in InfoCommand

* Fix merge

* Remove unnecessary case insensitive comparisons

* StringComparison.Ordinal gets unrolled by JIT

* Remove empty entries from tokenized ACL rule input

* Make ACL op keywords case insensitive per the PR feedback

* Check that Utf8Parser.TryParse consumes entire input

* Misc. adjustments

* Comparisons strings to uppercase
* Formatting adjustments
* Few unnecessary using directives in the files which PR touches

* Fix public field casing in Worker.cs

* Make Cluster node id comparisons case insensitive

* Make local node getters properties

* Makes it clear that the methods don't do extra "work"

* dotnet format

---------

Co-authored-by: Badrish Chandramouli <badrishc@microsoft.com>
Co-authored-by: Lukas Maas <lumaas@microsoft.com>
Co-authored-by: vazois <96085550+vazois@users.noreply.github.com>
  • Loading branch information
@PaulusParssinen @badrishc
@lmaas @vazois
4 people committed Apr 20, 2024
1 parent 515b6c1 commit 03611e1
Show file tree
Hide file tree
Showing 59 changed files with 530 additions and 599 deletions.
15 changes: 8 additions & 7 deletions libs/client/GarnetClientMetrics.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license.

using System;
using System.Globalization;
using Garnet.common;
using Garnet.networking;
using HdrHistogram;
Expand All @@ -28,13 +29,13 @@ private MetricsItem[] GetPercentiles(LongHistogram longHistogram, double scaling
if (histogram == null || histogram.TotalCount == 0)
return Array.Empty<MetricsItem>();

var _min = (histogram.GetValueAtPercentile(0) / scaling).ToString("N2");
var _5 = (histogram.GetValueAtPercentile(5) / scaling).ToString("N2");
var _50 = (histogram.GetValueAtPercentile(50) / scaling).ToString("N2");
var _mean = (histogram.GetMean() / scaling).ToString("N2");
var _95 = (histogram.GetValueAtPercentile(95) / scaling).ToString("N2");
var _99 = (histogram.GetValueAtPercentile(99) / scaling).ToString("N2");
var _999 = (histogram.GetValueAtPercentile(99.9) / scaling).ToString("N2");
var _min = (histogram.GetValueAtPercentile(0) / scaling).ToString("N2", CultureInfo.InvariantCulture);
var _5 = (histogram.GetValueAtPercentile(5) / scaling).ToString("N2", CultureInfo.InvariantCulture);
var _50 = (histogram.GetValueAtPercentile(50) / scaling).ToString("N2", CultureInfo.InvariantCulture);
var _mean = (histogram.GetMean() / scaling).ToString("N2", CultureInfo.InvariantCulture);
var _95 = (histogram.GetValueAtPercentile(95) / scaling).ToString("N2", CultureInfo.InvariantCulture);
var _99 = (histogram.GetValueAtPercentile(99) / scaling).ToString("N2", CultureInfo.InvariantCulture);
var _999 = (histogram.GetValueAtPercentile(99.9) / scaling).ToString("N2", CultureInfo.InvariantCulture);

MetricsItem[] percentiles = new MetricsItem[]
{
Expand Down
238 changes: 119 additions & 119 deletions libs/cluster/Server/ClusterConfig.cs
View file

Large diffs are not rendered by default.

40 changes: 20 additions & 20 deletions libs/cluster/Server/ClusterConfigSerializer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,27 +25,27 @@ public byte[] ToByteArray()
{
var worker = workers[i];
//40 bytes
writer.Write(worker.nodeid);
writer.Write(worker.Nodeid);

//16 bytes
writer.Write(worker.address);
writer.Write(worker.Address);

//29 bytes
writer.Write(worker.port);
writer.Write(worker.configEpoch);
writer.Write(worker.currentConfigEpoch);
writer.Write(worker.lastVotedConfigEpoch);
writer.Write((byte)worker.role);
writer.Write(worker.Port);
writer.Write(worker.ConfigEpoch);
writer.Write(worker.CurrentConfigEpoch);
writer.Write(worker.LastVotedConfigEpoch);
writer.Write((byte)worker.Role);

//1 byte
writer.Write(worker.replicaOfNodeId == null ? (byte)0 : (byte)1);
writer.Write(worker.ReplicaOfNodeId == null ? (byte)0 : (byte)1);

if (worker.replicaOfNodeId != null)
if (worker.ReplicaOfNodeId != null)
//40 bytes
writer.Write(worker.replicaOfNodeId);
writer.Write(worker.ReplicaOfNodeId);

//4 bytes
writer.Write(worker.replicationOffset);
writer.Write(worker.ReplicationOffset);
//1 byte
writer.Write(worker.hostname == null ? (byte)0 : (byte)1);
if (worker.hostname != null)
Expand Down Expand Up @@ -116,19 +116,19 @@ public static ClusterConfig FromByteArray(byte[] other)
var newWorkers = new Worker[numWorkers];
for (int i = 1; i < numWorkers; i++)
{
newWorkers[i].nodeid = reader.ReadString();
newWorkers[i].address = reader.ReadString();
newWorkers[i].port = reader.ReadInt32();
newWorkers[i].configEpoch = reader.ReadInt64();
newWorkers[i].currentConfigEpoch = reader.ReadInt64();
newWorkers[i].lastVotedConfigEpoch = reader.ReadInt64();
newWorkers[i].role = (NodeRole)reader.ReadByte();
newWorkers[i].Nodeid = reader.ReadString();
newWorkers[i].Address = reader.ReadString();
newWorkers[i].Port = reader.ReadInt32();
newWorkers[i].ConfigEpoch = reader.ReadInt64();
newWorkers[i].CurrentConfigEpoch = reader.ReadInt64();
newWorkers[i].LastVotedConfigEpoch = reader.ReadInt64();
newWorkers[i].Role = (NodeRole)reader.ReadByte();

byte isNull = reader.ReadByte();
if (isNull > 0)
newWorkers[i].replicaOfNodeId = reader.ReadString();
newWorkers[i].ReplicaOfNodeId = reader.ReadString();

newWorkers[i].replicationOffset = reader.ReadInt64();
newWorkers[i].ReplicationOffset = reader.ReadInt64();

isNull = reader.ReadByte();
if (isNull > 0)
Expand Down
26 changes: 13 additions & 13 deletions libs/cluster/Server/ClusterManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ public unsafe ClusterManager(ClusterProvider clusterProvider, ILogger logger = n
var config = ClusterUtils.ReadDevice(clusterConfigDevice, pool, logger);
currentConfig = ClusterConfig.FromByteArray(config);
// Used to update endpoint if it change when running inside a container.
if (address != currentConfig.GetLocalNodeIp() || opts.Port != currentConfig.GetLocalNodePort())
if (address != currentConfig.LocalNodeIp || opts.Port != currentConfig.LocalNodePort)
{
logger?.LogInformation(
"Updating local Endpoint: From {currentConfig.GetLocalNodeIp()}:{currentConfig.GetLocalNodePort()} to {address}:{opts.Port}",
currentConfig.GetLocalNodeIp(),
currentConfig.GetLocalNodePort(),
currentConfig.LocalNodeIp,
currentConfig.LocalNodePort,
address,
opts.Port);
}
Expand Down Expand Up @@ -142,14 +142,14 @@ private void InitLocal(string address, int port, bool recoverConfig)
{
var conf = currentConfig;
TryInitializeLocalWorker(
conf.GetLocalNodeId(),
conf.LocalNodeId,
address,
port,
configEpoch: conf.GetLocalNodeConfigEpoch(),
currentConfigEpoch: conf.GetLocalNodeCurrentConfigEpoch(),
lastVotedConfigEpoch: conf.GetLocalNodeLastVotedEpoch(),
role: conf.GetLocalNodeRole(),
replicaOfNodeId: conf.GetLocalNodePrimaryId(),
configEpoch: conf.LocalNodeConfigEpoch,
currentConfigEpoch: conf.LocalNodeCurrentConfigEpoch,
lastVotedConfigEpoch: conf.LocalNodeLastVotedEpoch,
role: conf.LocalNodeRole,
replicaOfNodeId: conf.LocalNodePrimaryId,
hostname: Format.GetHostName());
}
else
Expand Down Expand Up @@ -179,7 +179,7 @@ public string GetInfo()
$"cluster_known_nodes:{current.NumWorkers}\r\n" +
$"cluster_size:{current.GetPrimaryCount()}\r\n" +
$"cluster_current_epoch:{current.GetMaxConfigEpoch()}\r\n" +
$"cluster_my_epoch:{current.GetLocalNodeConfigEpoch()}\r\n" +
$"cluster_my_epoch:{current.LocalNodeConfigEpoch}\r\n" +
$"cluster_stats_messages_sent:0\r\n" +
$"cluster_stats_messages_received:0\r\n";
return ClusterInfo;
Expand Down Expand Up @@ -342,17 +342,17 @@ public bool AuthorizeFailover(string requestingNodeId, long requestedEpoch, byte
var current = currentConfig;

//If I am not a primary or I do not have any assigned slots cannot vote
var role = current.GetLocalNodeRole();
var role = current.LocalNodeRole;
if (role != NodeRole.PRIMARY || current.HasAssignedSlots(1))
return false;

//if I already voted for this epoch return
if (current.GetLocalNodeLastVotedEpoch() == requestedEpoch)
if (current.LocalNodeLastVotedEpoch == requestedEpoch)
return false;

//Requesting node has to be a known replica node
var requestingNodeWorker = current.GetWorkerFromNodeId(requestingNodeId);
if (requestingNodeWorker.role == NodeRole.UNASSIGNED)
if (requestingNodeWorker.Role == NodeRole.UNASSIGNED)
return false;

//Check if configEpoch for claimed slots is lower than the config of the requested epoch.
Expand Down
22 changes: 11 additions & 11 deletions libs/cluster/Server/ClusterManagerSlotState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public bool TryPrepareSlotForMigration(int slot, string nodeid, out ReadOnlySpan
return false;
}

if (current.GetLocalNodeId().Equals(nodeid))
if (current.LocalNodeId.Equals(nodeid, StringComparison.OrdinalIgnoreCase))
{
errorMessage = CmdStrings.RESP_ERR_GENERIC_MIGRATE_TO_MYSELF;
return false;
Expand Down Expand Up @@ -167,7 +167,7 @@ public bool TryPrepareSlotsForMigration(HashSet<int> slots, string nodeid, out R
}

//Check if nodeid is different from local node
if (current.GetLocalNodeId().Equals(nodeid))
if (current.LocalNodeId.Equals(nodeid, StringComparison.OrdinalIgnoreCase))
{
errorMessage = CmdStrings.RESP_ERR_GENERIC_MIGRATE_TO_MYSELF;
return false;
Expand Down Expand Up @@ -232,9 +232,9 @@ public bool TryPrepareSlotForImport(int slot, string nodeid, out ReadOnlySpan<by
return false;
}

if (current.GetLocalNodeRole() != NodeRole.PRIMARY)
if (current.LocalNodeRole != NodeRole.PRIMARY)
{
errorMessage = Encoding.ASCII.GetBytes($"ERR Importing node {current.GetLocalNodeRole()} is not a master node.");
errorMessage = Encoding.ASCII.GetBytes($"ERR Importing node {current.LocalNodeRole} is not a master node.");
return false;
}

Expand All @@ -245,7 +245,7 @@ public bool TryPrepareSlotForImport(int slot, string nodeid, out ReadOnlySpan<by
}

string sourceNodeId = current.GetNodeIdFromSlot((ushort)slot);
if (sourceNodeId == null || !sourceNodeId.Equals(nodeid))
if (sourceNodeId == null || !sourceNodeId.Equals(nodeid, StringComparison.OrdinalIgnoreCase))
{
errorMessage = Encoding.ASCII.GetBytes($"ERR Slot {slot} is not owned by {nodeid}");
return false;
Expand Down Expand Up @@ -289,9 +289,9 @@ public bool TryPrepareSlotsForImport(HashSet<int> slots, string nodeid, out Read
}

// Check local node is a primary
if (current.GetLocalNodeRole() != NodeRole.PRIMARY)
if (current.LocalNodeRole != NodeRole.PRIMARY)
{
errorMessage = Encoding.ASCII.GetBytes($"ERR Importing node {current.GetLocalNodeRole()} is not a master node.");
errorMessage = Encoding.ASCII.GetBytes($"ERR Importing node {current.LocalNodeRole} is not a master node.");
return false;
}

Expand All @@ -307,7 +307,7 @@ public bool TryPrepareSlotsForImport(HashSet<int> slots, string nodeid, out Read

// Check if node is owned by node
var sourceNodeId = current.GetNodeIdFromSlot((ushort)slot);
if (sourceNodeId == null || !sourceNodeId.Equals(nodeid))
if (sourceNodeId == null || !sourceNodeId.Equals(nodeid, StringComparison.OrdinalIgnoreCase))
{
errorMessage = Encoding.ASCII.GetBytes($"ERR Slot {slot} is not owned by {nodeid}");
return false;
Expand Down Expand Up @@ -365,9 +365,9 @@ public bool TryPrepareSlotForOwnershipChange(int slot, string nodeid, out ReadOn
}
else if (current.GetState((ushort)slot) is SlotState.IMPORTING)
{
if (!current.GetLocalNodeId().Equals(nodeid))
if (!current.LocalNodeId.Equals(nodeid, StringComparison.OrdinalIgnoreCase))
{
errorMesage = Encoding.ASCII.GetBytes($"ERR Input nodeid {nodeid} different from local nodeid {CurrentConfig.GetLocalNodeId()}.");
errorMesage = Encoding.ASCII.GetBytes($"ERR Input nodeid {nodeid} different from local nodeid {CurrentConfig.LocalNodeId}.");
return false;
}

Expand Down Expand Up @@ -408,7 +408,7 @@ public bool TryPrepareSlotsForOwnershipChange(HashSet<int> slots, string nodeid,
}

var newConfig = currentConfig.UpdateMultiSlotState(slots, workerId, SlotState.STABLE);
if (current.GetLocalNodeId().Equals(nodeid)) newConfig = newConfig.BumpLocalNodeConfigEpoch();
if (current.LocalNodeId.Equals(nodeid, StringComparison.OrdinalIgnoreCase)) newConfig = newConfig.BumpLocalNodeConfigEpoch();
if (Interlocked.CompareExchange(ref currentConfig, newConfig, current) == current)
break;
}
Expand Down
24 changes: 12 additions & 12 deletions libs/cluster/Server/ClusterManagerWorkerState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public bool TryRemoveWorker(string nodeid, int expirySeconds, out ReadOnlySpan<b
{
var current = currentConfig;

if (current.GetLocalNodeId().Equals(nodeid))
if (current.LocalNodeId.Equals(nodeid, StringComparison.OrdinalIgnoreCase))
{
errorMessage = CmdStrings.RESP_ERR_GENERIC_CANNOT_FORGET_MYSELF;
return false;
Expand All @@ -69,7 +69,7 @@ public bool TryRemoveWorker(string nodeid, int expirySeconds, out ReadOnlySpan<b
return false;
}

if (current.GetLocalNodeRole() == NodeRole.REPLICA && current.GetLocalNodePrimaryId().Equals(nodeid))
if (current.LocalNodeRole == NodeRole.REPLICA && current.LocalNodePrimaryId.Equals(nodeid, StringComparison.OrdinalIgnoreCase))
{
errorMessage = CmdStrings.RESP_ERR_GENERIC_CANNOT_FORGET_MY_PRIMARY;
return false;
Expand Down Expand Up @@ -106,13 +106,13 @@ public ReadOnlySpan<byte> TryReset(bool soft, int expirySeconds = 60)
while (true)
{
var current = currentConfig;
var newNodeId = soft ? current.GetLocalNodeId() : Generator.CreateHexId();
var address = current.GetLocalNodeIp();
var port = current.GetLocalNodePort();
var newNodeId = soft ? current.LocalNodeId : Generator.CreateHexId();
var address = current.LocalNodeIp;
var port = current.LocalNodePort;

var configEpoch = soft ? current.GetLocalNodeConfigEpoch() : 0;
var currentConfigEpoch = soft ? current.GetLocalNodeCurrentConfigEpoch() : 0;
var lastVotedConfigEpoch = soft ? current.GetLocalNodeLastVotedEpoch() : 0;
var configEpoch = soft ? current.LocalNodeConfigEpoch : 0;
var currentConfigEpoch = soft ? current.LocalNodeCurrentConfigEpoch : 0;
var lastVotedConfigEpoch = soft ? current.LocalNodeLastVotedEpoch : 0;

var expiry = DateTimeOffset.UtcNow.Ticks + TimeSpan.FromSeconds(expirySeconds).Ticks;
foreach (var nodeId in current.GetRemoteNodeIds())
Expand Down Expand Up @@ -154,17 +154,17 @@ public bool TryAddReplica(string nodeid, bool force, ref bool recovering, out Re
while (true)
{
var current = CurrentConfig;
if (current.GetLocalNodeId().Equals(nodeid))
if (current.LocalNodeId.Equals(nodeid, StringComparison.OrdinalIgnoreCase))
{
errorMessage = CmdStrings.RESP_ERR_GENERIC_CANNOT_REPLICATE_SELF;
logger?.LogError(Encoding.ASCII.GetString(errorMessage));
return false;
}

if (!force && current.GetLocalNodeRole() != NodeRole.PRIMARY)
if (!force && current.LocalNodeRole != NodeRole.PRIMARY)
{
logger?.LogError("ERR I am already replica of {localNodePrimaryId}", current.GetLocalNodePrimaryId());
errorMessage = Encoding.ASCII.GetBytes($"ERR I am already replica of {current.GetLocalNodePrimaryId()}.");
logger?.LogError("ERR I am already replica of {localNodePrimaryId}", current.LocalNodePrimaryId);
errorMessage = Encoding.ASCII.GetBytes($"ERR I am already replica of {current.LocalNodePrimaryId}.");
return false;
}

Expand Down
8 changes: 4 additions & 4 deletions libs/cluster/Server/ClusterProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ public void Dispose()

/// <inheritdoc />
public bool IsReplica()
=> clusterManager?.CurrentConfig.GetLocalNodeRole() == NodeRole.REPLICA || replicationManager?.recovering == true;
=> clusterManager?.CurrentConfig.LocalNodeRole == NodeRole.REPLICA || replicationManager?.recovering == true;

/// <inheritdoc />
public void ResetGossipStats()
Expand Down Expand Up @@ -156,7 +156,7 @@ public void SafeTruncateAOF(StoreType storeType, bool full, long CheckpointCover
// Used to delete old checkpoints and cleanup and also cleanup during attachment to new primary
replicationManager.AddCheckpointEntry(entry, storeType, full);

if (clusterManager.CurrentConfig.GetLocalNodeRole() == NodeRole.PRIMARY)
if (clusterManager.CurrentConfig.LocalNodeRole == NodeRole.PRIMARY)
_ = replicationManager.SafeTruncateAof(CheckpointCoveredAofAddress);
else
{
Expand All @@ -174,7 +174,7 @@ public void SafeTruncateAOF(StoreType storeType, bool full, long CheckpointCover
public void OnCheckpointInitiated(out long CheckpointCoveredAofAddress)
{
Debug.Assert(serverOptions.EnableCluster);
if (serverOptions.EnableAOF && clusterManager.CurrentConfig.GetLocalNodeRole() == NodeRole.REPLICA)
if (serverOptions.EnableAOF && clusterManager.CurrentConfig.LocalNodeRole == NodeRole.REPLICA)
CheckpointCoveredAofAddress = replicationManager.ReplicationOffset;
else
CheckpointCoveredAofAddress = storeWrapper.appendOnlyFile.TailAddress;
Expand All @@ -188,7 +188,7 @@ public MetricsItem[] GetReplicationInfo()
var clusterEnabled = serverOptions.EnableCluster;
var config = clusterEnabled ? clusterManager.CurrentConfig : null;
var replicaInfo = clusterEnabled ? replicationManager.GetReplicaInfo() : null;
var role = clusterEnabled ? config.GetLocalNodeRole() : NodeRole.PRIMARY;
var role = clusterEnabled ? config.LocalNodeRole : NodeRole.PRIMARY;
var replication_offset = !clusterEnabled ? "N/A" : replicationManager.ReplicationOffset.ToString();
var replication_offset2 = !clusterEnabled ? "N/A" : replicationManager.ReplicationOffset2.ToString();

Expand Down
Loading

0 comments on commit 03611e1

Please sign in to comment.