From 753cb8cda8db68d92223bd94940bfe193d21a4f4 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 10 Aug 2026 11:02:09 -0700 Subject: [PATCH] master: stop copying the cluster to name it (#10700) * topology: name a node's volumes without copying them ToVolumeLocations reads a volume id off every volume in the cluster, and got there through GetVolumes, which copies a whole storage.VolumeInfo per volume to be read for four bytes of it. Every client that connects asks for this. At 800k volumes the walk goes from 94.6MB to 16.0MB, which is the ids themselves. * master: log why a client send failed, not what was sent The message names every volume on a newly connected node, so a client going away had the master format a protobuf that size into text -- through the one log level that is always on. The error is the part worth having. --- weed/server/master_grpc_server.go | 5 ++++- weed/topology/data_node.go | 11 +++++++++++ weed/topology/disk.go | 12 ++++++++++++ weed/topology/topology_info.go | 6 +----- 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/weed/server/master_grpc_server.go b/weed/server/master_grpc_server.go index 623682b28..2f2a89b81 100644 --- a/weed/server/master_grpc_server.go +++ b/weed/server/master_grpc_server.go @@ -474,7 +474,10 @@ func (ms *MasterServer) KeepConnected(stream master_pb.Seaweed_KeepConnectedServ select { case message := <-messageChan: if err := stream.Send(message); err != nil { - glog.V(0).Infof("=> client %v: %+v", clientName, message) + // The error, not the message: it carries every volume id on a + // newly connected node, and formatting a proto that size to + // say a client went away costs more than the send did. + glog.V(0).Infof("=> client %v: %v", clientName, err) return err } case <-ticker.C: diff --git a/weed/topology/data_node.go b/weed/topology/data_node.go index 19c8b2007..78daf2276 100644 --- a/weed/topology/data_node.go +++ b/weed/topology/data_node.go @@ -219,6 +219,17 @@ func (dn *DataNode) AdjustDiskUsageBytes(diskTotalBytes, diskFreeBytes map[strin } } +// AppendVolumeIds appends the ids of this node's volumes to dst, without +// copying the volume records to read them. +func (dn *DataNode) AppendVolumeIds(dst []uint32) []uint32 { + dn.RLock() + defer dn.RUnlock() + for _, c := range dn.children { + dst = c.(*Disk).AppendVolumeIds(dst) + } + return dst +} + func (dn *DataNode) GetVolumes() (ret []storage.VolumeInfo) { dn.RLock() defer dn.RUnlock() diff --git a/weed/topology/disk.go b/weed/topology/disk.go index 1ec7f93fc..5b1333397 100644 --- a/weed/topology/disk.go +++ b/weed/topology/disk.go @@ -277,6 +277,18 @@ func (d *Disk) GetVolumes() []storage.VolumeInfo { return d.AppendVolumes(make([]storage.VolumeInfo, 0, d.VolumeCount())) } +// AppendVolumeIds appends the ids of the disk's volumes to dst. Callers that +// only need to name volumes use this rather than AppendVolumes, which copies +// a whole record per volume to be read for four bytes of it. +func (d *Disk) AppendVolumeIds(dst []uint32) []uint32 { + d.RLock() + defer d.RUnlock() + for id := range d.volumes { + dst = append(dst, uint32(id)) + } + return dst +} + // AppendVolumes appends the disk's volumes to dst, so a caller gathering // several disks fills one slice instead of concatenating a copy per disk. func (d *Disk) AppendVolumes(dst []storage.VolumeInfo) []storage.VolumeInfo { diff --git a/weed/topology/topology_info.go b/weed/topology/topology_info.go index c60b68329..f8df207b0 100644 --- a/weed/topology/topology_info.go +++ b/weed/topology/topology_info.go @@ -100,11 +100,7 @@ func (t *Topology) ToVolumeLocations() (volumeLocations []*master_pb.VolumeLocat DataCenter: dn.GetDataCenterId(), GrpcPort: uint32(dn.GrpcPort), } - dnVolumes := dn.GetVolumes() - volumeLocation.NewVids = make([]uint32, 0, len(dnVolumes)) - for _, v := range dnVolumes { - volumeLocation.NewVids = append(volumeLocation.NewVids, uint32(v.Id)) - } + volumeLocation.NewVids = dn.AppendVolumeIds(nil) // A single EC volume's shards can live on multiple disks of // one DataNode, so GetEcShards returns per-(vid,disk) entries. // Dedupe so the snapshot carries each vid once.