Skip to content

Commit dafb35f

Browse files
vazoisCopilot
vazois
and
Copilot
authored
Fix CLUSTER NODES and CLUSTER SHARDS metadata output (#1657)
* Fix CLUSTER NODES and CLUSTER SHARDS metadata output (#1650) - CLUSTER NODES: Only append ,hostname when hostname is non-empty - CLUSTER SHARDS: Replace 'address' field with Redis-compatible 'ip', 'endpoint', and optional 'hostname' fields - CLUSTER SHARDS: Honor ClusterPreferredEndpointType for endpoint field (ip address when Ip, hostname when Hostname) - CLUSTER SHARDS: Fix role output to use 'master'/'slave' instead of enum names 'PRIMARY'/'REPLICA' - Update NodeInfo struct and ClusterShards parser in test utilities for dynamic key-value field parsing - Add ClusterShardsTest and ClusterNodesHostnameTest unit tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Refactor CLUSTER SLOTS/SHARDS to use StringBuilder Replace string concatenation with StringBuilder in GetShardsInfo, GetSlotsInfo, and their helper methods (AppendFormattedSlotInfo, AppendNodeNetworkingInfo) to reduce intermediate string allocations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR review comments and fix endpoint validation - Fix endpoint validation (Options.cs): allow cluster-announce-ip when bind address is 0.0.0.0/:: (wildcard), since the server listens on all interfaces. Only require port match in that case. - Handle ClusterPreferredEndpointType.Unknown in CLUSTER SHARDS: set endpoint to '?' consistent with CLUSTER SLOTS and redirects. - Add ClusterShardsTest cases for Unknown endpoint type. - Improve ClusterNodesHostnameTest to handle both hostname-present and hostname-absent branches. - Add even-length guard and explicit role validation in ClusterShards test parser. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent dc0a9d0 commit dafb35f

File tree

5 files changed

+243
-81
lines changed

5 files changed

+243
-81
lines changed

‎libs/cluster/Server/ClusterConfig.cs‎

Lines changed: 94 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -544,7 +544,12 @@ private void GetNodeInfo(ushort workerId, ConnectionInfo info, StringBuilder nod
544544
_ = nodeInfoStringBuilder
545545
.Append(workers[workerId].Nodeid).Append(' ')
546546
.Append(workers[workerId].Address).Append(':').Append(workers[workerId].Port)
547-
.Append('@').Append(workers[workerId].Port + 10000).Append(',').Append(workers[workerId].hostname).Append(' ')
547+
.Append('@').Append(workers[workerId].Port + 10000);
548+
549+
if (!string.IsNullOrEmpty(workers[workerId].hostname))
550+
nodeInfoStringBuilder.Append(',').Append(workers[workerId].hostname);
551+
552+
_ = nodeInfoStringBuilder.Append(' ')
548553
.Append(workerId == 1 ? "myself," : "")
549554
.Append(workers[workerId].Role == NodeRole.PRIMARY ? "master" : "slave").Append(' ')
550555
.Append(workers[workerId].Role == NodeRole.REPLICA ? workers[workerId].ReplicaOfNodeId : '-').Append(' ')
@@ -657,73 +662,95 @@ public List<int> GetWorkerReplicas(int workerId)
657662
/// </summary>
658663
/// <param name="clusterConnection"></param>
659664
/// <returns>RESP formatted string</returns>
660-
public string GetShardsInfo(GarnetClusterConnectionStore clusterConnection)
665+
public string GetShardsInfo(GarnetClusterConnectionStore clusterConnection, ClusterPreferredEndpointType preferredEndpointType)
661666
{
662-
var shardsInfo = "";
667+
var sb = new StringBuilder();
663668
var shardCount = 0;
669+
var shardsStart = sb.Length;
664670
for (ushort i = 1; i <= NumWorkers; i++)
665671
{
666672
if (workers[i].Role == NodeRole.PRIMARY)
667673
{
668674
var shardRanges = GetShardRanges(i);
669675
var replicaWorkerIds = GetWorkerReplicas(i);
670-
shardsInfo += CreateFormattedShardInfo(i, shardRanges, replicaWorkerIds);
676+
AppendFormattedShardInfo(sb, i, shardRanges, replicaWorkerIds);
671677
shardCount++;
672678
}
673679
}
674-
shardsInfo = $"*{shardCount}\r\n" + shardsInfo;
675-
return shardsInfo;
680+
sb.Insert(shardsStart, $"*{shardCount}\r\n");
681+
return sb.ToString();
676682

677-
string CreateFormattedShardInfo(int primaryWorkerId, List<(ushort, ushort)> shardRanges, List<int> replicaWorkerIds)
683+
void AppendFormattedShardInfo(StringBuilder sb, int primaryWorkerId, List<(ushort, ushort)> shardRanges, List<int> replicaWorkerIds)
678684
{
679-
var shardInfo = $"*4\r\n";
680-
shardInfo += $"$5\r\nslots\r\n";
681-
shardInfo += $"*{shardRanges.Count * 2}\r\n";
685+
sb.Append("*4\r\n");
686+
sb.Append("$5\r\nslots\r\n");
687+
sb.Append('*').Append(shardRanges.Count * 2).Append("\r\n");
682688
for (var i = 0; i < shardRanges.Count; i++)
683689
{
684690
var range = shardRanges[i];
685-
shardInfo += $":{range.Item1}\r\n";
686-
shardInfo += $":{range.Item2}\r\n";
691+
sb.Append(':').Append(range.Item1).Append("\r\n");
692+
sb.Append(':').Append(range.Item2).Append("\r\n");
687693
}
688694

689-
shardInfo += $"$5\r\nnodes\r\n";
690-
shardInfo += $"*{1 + replicaWorkerIds.Count}\r\n";
695+
sb.Append("$5\r\nnodes\r\n");
696+
sb.Append('*').Append(1 + replicaWorkerIds.Count).Append("\r\n");
691697
if (primaryWorkerId == 1)
692-
shardInfo += CreateFormattedNodeInfo(primaryWorkerId, true);
698+
AppendFormattedNodeInfo(sb, primaryWorkerId, true);
693699
else
694700
{
695701
_ = clusterConnection.GetConnectionInfo(workers[primaryWorkerId].Nodeid, out var info);
696-
shardInfo += CreateFormattedNodeInfo(primaryWorkerId, info.connected);
702+
AppendFormattedNodeInfo(sb, primaryWorkerId, info.connected);
697703
}
698704
foreach (var id in replicaWorkerIds)
699705
{
700706
_ = clusterConnection.GetConnectionInfo(workers[id].Nodeid, out var info);
701-
shardInfo += CreateFormattedNodeInfo(id, info.connected);
707+
AppendFormattedNodeInfo(sb, id, info.connected);
702708
}
709+
}
703710

704-
return shardInfo;
711+
void AppendFormattedNodeInfo(StringBuilder sb, int workerId, bool connected)
712+
{
713+
var ip = workers[workerId].Address;
714+
var hostname = workers[workerId].hostname;
715+
var hasHostname = !string.IsNullOrEmpty(hostname);
716+
var role = workers[workerId].Role == NodeRole.PRIMARY ? "master" : "slave";
705717

706-
string CreateFormattedNodeInfo(int workerId, bool connected)
718+
var endpoint = preferredEndpointType switch
719+
{
720+
ClusterPreferredEndpointType.Hostname => hasHostname ? hostname : "?",
721+
ClusterPreferredEndpointType.Unknown => "?",
722+
_ => ip,
723+
};
724+
725+
// Base fields: id(2) + port(2) + ip(2) + endpoint(2) + role(2) + replication-offset(2) + health(2) = 14
726+
// Optional: hostname(2) = +2
727+
var fieldCount = hasHostname ? 16 : 14;
728+
729+
sb.Append('*').Append(fieldCount).Append("\r\n");
730+
sb.Append("$2\r\nid\r\n");
731+
sb.Append("$40\r\n").Append(workers[workerId].Nodeid).Append("\r\n");
732+
sb.Append("$4\r\nport\r\n");
733+
sb.Append(':').Append(workers[workerId].Port).Append("\r\n");
734+
sb.Append("$2\r\nip\r\n");
735+
sb.Append('$').Append(ip.Length).Append("\r\n").Append(ip).Append("\r\n");
736+
sb.Append("$8\r\nendpoint\r\n");
737+
sb.Append('$').Append(endpoint.Length).Append("\r\n").Append(endpoint).Append("\r\n");
738+
if (hasHostname)
707739
{
708-
var nodeInfo = "*12\r\n";
709-
nodeInfo += "$2\r\nid\r\n";
710-
nodeInfo += $"$40\r\n{workers[workerId].Nodeid}\r\n";
711-
nodeInfo += "$4\r\nport\r\n";
712-
nodeInfo += $":{workers[workerId].Port}\r\n";
713-
nodeInfo += "$7\r\naddress\r\n";
714-
nodeInfo += $"${workers[workerId].Address.Length}\r\n{workers[workerId].Address}\r\n";
715-
nodeInfo += "$4\r\nrole\r\n";
716-
nodeInfo += $"${workers[workerId].Role.ToString().Length}\r\n{workers[workerId].Role}\r\n";
717-
nodeInfo += "$18\r\nreplication-offset\r\n";
718-
nodeInfo += $":{workers[workerId].ReplicationOffset}\r\n";
719-
nodeInfo += "$6\r\nhealth\r\n";
720-
nodeInfo += connected ? "$6\r\nonline\r\n" : "$7\r\noffline\r\n";
721-
return nodeInfo;
740+
sb.Append("$8\r\nhostname\r\n");
741+
sb.Append('$').Append(hostname.Length).Append("\r\n").Append(hostname).Append("\r\n");
722742
}
743+
sb.Append("$4\r\nrole\r\n");
744+
sb.Append('$').Append(role.Length).Append("\r\n").Append(role).Append("\r\n");
745+
sb.Append("$18\r\nreplication-offset\r\n");
746+
sb.Append(':').Append(workers[workerId].ReplicationOffset).Append("\r\n");
747+
sb.Append("$6\r\nhealth\r\n");
748+
sb.Append(connected ? "$6\r\nonline\r\n" : "$7\r\noffline\r\n");
723749
}
724750
}
725751

726-
private string CreateFormattedSlotInfo(
752+
private void AppendFormattedSlotInfo(
753+
StringBuilder sb,
727754
int slotStart,
728755
int slotEnd,
729756
string ipAddress,
@@ -734,24 +761,23 @@ private string CreateFormattedSlotInfo(
734761
ClusterPreferredEndpointType preferredEndpointType)
735762
{
736763
int countA = replicaIds.Count == 0 ? 3 : 3 + replicaIds.Count;
737-
var rangeInfo = $"*{countA}\r\n";
764+
sb.Append('*').Append(countA).Append("\r\n");
738765

739-
rangeInfo += $":{slotStart}\r\n";
740-
rangeInfo += $":{slotEnd}\r\n";
766+
sb.Append(':').Append(slotStart).Append("\r\n");
767+
sb.Append(':').Append(slotEnd).Append("\r\n");
741768

742-
rangeInfo += CreateNodeNetworkingInfo(ipAddress, port, nodeid, hostname, preferredEndpointType);
769+
AppendNodeNetworkingInfo(sb, ipAddress, port, nodeid, hostname, preferredEndpointType);
743770

744771
foreach (var replicaId in replicaIds)
745772
{
746773
var (replicaIp, replicaPort) = GetWorkerAddressFromNodeId(replicaId);
747774
var replicaHostname = GetHostNameFromNodeId(replicaId);
748-
rangeInfo += CreateNodeNetworkingInfo(replicaIp, replicaPort, replicaId, replicaHostname, preferredEndpointType);
775+
AppendNodeNetworkingInfo(sb, replicaIp, replicaPort, replicaId, replicaHostname, preferredEndpointType);
749776
}
750-
751-
return rangeInfo;
752777
}
753778

754-
private string CreateNodeNetworkingInfo(
779+
private void AppendNodeNetworkingInfo(
780+
StringBuilder sb,
755781
string ipAddress,
756782
int port,
757783
string nodeid,
@@ -768,58 +794,59 @@ private string CreateNodeNetworkingInfo(
768794
// "ip" is included if preferred endpoint type != Ip
769795
// "hostname" is included if preferred endpoint type != Hostname AND hostname is not null or empty
770796

771-
var sb = new StringBuilder();
772797
sb.Append("*4\r\n");
773798
var isNullOrEmptyHostname = string.IsNullOrEmpty(hostname);
774799

775800
switch (preferredEndpointType)
776801
{
777802
case ClusterPreferredEndpointType.Ip:
778-
sb.Append(FormatValueOrNull(ipAddress))
779-
.Append(':').Append(port).Append("\r\n")
803+
AppendValueOrNull(sb, ipAddress);
804+
sb.Append(':').Append(port).Append("\r\n")
780805
.Append('$').Append(nodeid.Length).Append("\r\n").Append(nodeid).Append("\r\n")
781806
.Append('*').Append(isNullOrEmptyHostname ? 0 : 2).Append("\r\n");
782807

783808
if (!isNullOrEmptyHostname)
784809
{
785-
sb.Append("$8\r\nhostname\r\n")
786-
.Append(FormatValueOrNull(hostname));
810+
sb.Append("$8\r\nhostname\r\n");
811+
AppendValueOrNull(sb, hostname);
787812
}
788813

789-
return sb.ToString();
814+
break;
790815
case ClusterPreferredEndpointType.Hostname:
791816
var hostnameForResp = isNullOrEmptyHostname ? "?" : hostname;
792817

793-
sb.Append(FormatValueOrNull(hostnameForResp))
794-
.Append(':').Append(port).Append("\r\n")
818+
AppendValueOrNull(sb, hostnameForResp);
819+
sb.Append(':').Append(port).Append("\r\n")
795820
.Append('$').Append(nodeid.Length).Append("\r\n").Append(nodeid).Append("\r\n")
796821
.Append('*').Append(2).Append("\r\n")
797-
.Append("$2\r\nip\r\n")
798-
.Append(FormatValueOrNull(ipAddress));
822+
.Append("$2\r\nip\r\n");
823+
AppendValueOrNull(sb, ipAddress);
799824

800-
return sb.ToString();
825+
break;
801826
case ClusterPreferredEndpointType.Unknown:
802827
default:
803-
sb.Append(FormatValueOrNull(null))
804-
.Append(':').Append(port).Append("\r\n")
828+
AppendValueOrNull(sb, null);
829+
sb.Append(':').Append(port).Append("\r\n")
805830
.Append('$').Append(nodeid.Length).Append("\r\n").Append(nodeid).Append("\r\n")
806831
.Append('*').Append(isNullOrEmptyHostname ? 2 : 4).Append("\r\n")
807-
.Append("$2\r\nip\r\n")
808-
.Append(FormatValueOrNull(ipAddress));
832+
.Append("$2\r\nip\r\n");
833+
AppendValueOrNull(sb, ipAddress);
809834

810835
if (!isNullOrEmptyHostname)
811836
{
812837
sb.Append("$8\r\nhostname\r\n");
813-
sb.Append(FormatValueOrNull(hostname));
838+
AppendValueOrNull(sb, hostname);
814839
}
815840

816-
return sb.ToString();
841+
break;
817842
}
818843

819-
static string FormatValueOrNull(string value)
844+
static void AppendValueOrNull(StringBuilder sb, string value)
820845
{
821-
if (string.IsNullOrEmpty(value)) return CmdStrings.GenericNullValue; // "$-1\r\n"
822-
return $"${value.Length}\r\n{value}\r\n";
846+
if (string.IsNullOrEmpty(value))
847+
sb.Append(CmdStrings.GenericNullValue); // "$-1\r\n"
848+
else
849+
sb.Append('$').Append(value.Length).Append("\r\n").Append(value).Append("\r\n");
823850
}
824851
}
825852

@@ -836,10 +863,11 @@ static string FormatValueOrNull(string value)
836863
/// <returns>Formatted string.</returns>
837864
public string GetSlotsInfo(ClusterPreferredEndpointType preferredEndpointType)
838865
{
839-
string completeSlotInfo = "";
866+
var sb = new StringBuilder();
840867
int slotRanges = 0;
841868
int slotStart;
842869
int slotEnd;
870+
var slotsStart = sb.Length;
843871

844872
for (slotStart = 0; slotStart < slotMap.Length; slotStart++)
845873
{
@@ -859,14 +887,13 @@ public string GetSlotsInfo(ClusterPreferredEndpointType preferredEndpointType)
859887
var hostname = workers[currSlotWorkerId].hostname;
860888
var replicas = GetReplicaIds(nodeid);
861889
slotEnd--;
862-
completeSlotInfo += CreateFormattedSlotInfo(slotStart, slotEnd, address, port, nodeid, hostname, replicas, preferredEndpointType);
890+
AppendFormattedSlotInfo(sb, slotStart, slotEnd, address, port, nodeid, hostname, replicas, preferredEndpointType);
863891
slotRanges++;
864892
slotStart = slotEnd;
865893
}
866-
completeSlotInfo = $"*{slotRanges}\r\n" + completeSlotInfo;
867-
//Console.WriteLine(completeSlotInfo);
894+
sb.Insert(slotsStart, $"*{slotRanges}\r\n");
868895

869-
return completeSlotInfo;
896+
return sb.ToString();
870897
}
871898

872899
/// <summary>

‎libs/cluster/Session/RespClusterBasicCommands.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,8 @@ private bool NetworkClusterShards(out bool invalidParameters)
341341
return true;
342342
}
343343

344-
var shardsInfo = clusterProvider.clusterManager.CurrentConfig.GetShardsInfo(clusterProvider.clusterManager.clusterConnectionStore);
344+
var preferredType = clusterProvider.serverOptions.ClusterPreferredEndpointType;
345+
var shardsInfo = clusterProvider.clusterManager.CurrentConfig.GetShardsInfo(clusterProvider.clusterManager.clusterConnectionStore, preferredType);
345346
while (!RespWriteUtils.TryWriteAsciiDirect(shardsInfo, ref dcurr, dend))
346347
SendAndReset();
347348

‎libs/host/Configuration/Options.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -760,7 +760,12 @@ public GarnetServerOptions GetServerOptions(ILogger logger = null)
760760
{
761761
ClusterAnnouncePort = ClusterAnnouncePort == 0 ? Port : ClusterAnnouncePort;
762762
clusterAnnounceEndpoint = Format.TryCreateEndpoint(ClusterAnnounceIp, ClusterAnnouncePort, tryConnect: false, logger: logger).GetAwaiter().GetResult();
763-
if (clusterAnnounceEndpoint == null || !endpoints.Any(endpoint => endpoint.Equals(clusterAnnounceEndpoint[0])))
763+
if (clusterAnnounceEndpoint == null || !endpoints.Any(endpoint =>
764+
endpoint is IPEndPoint listenEp && clusterAnnounceEndpoint[0] is IPEndPoint announceEp &&
765+
listenEp.Port == announceEp.Port &&
766+
(listenEp.Address.Equals(announceEp.Address) ||
767+
listenEp.Address.Equals(IPAddress.Any) ||
768+
listenEp.Address.Equals(IPAddress.IPv6Any))))
764769
throw new GarnetException("Cluster announce endpoint does not match list of listen endpoints provided!");
765770
}
766771

0 commit comments

Comments
 (0)