Skip to content

refactor: 标识符命名按 Go 惯例整理(含一处错误契约缺陷修复) - #253

Merged
NeverENG merged 7 commits into
mainfrom
refactor/go-idiomatic-naming
Aug 12, 2026
Merged

refactor: 标识符命名按 Go 惯例整理(含一处错误契约缺陷修复)#253
NeverENG merged 7 commits into
mainfrom
refactor/go-idiomatic-naming

Conversation

@NeverENG

Copy link
Copy Markdown
Collaborator

承接 #250(目录与包布局)。本批处理标识符命名,并修掉一处随之暴露的真实缺陷。

1. 错误契约(不只是风格)

「key 不存在」此前在 memtable.go 四处各自 errors.New 一个新对象,调用方因此无法把它与读盘失败等真实故障区分开——两者在上层看来都只是「某个 error」,只能一视同仁当作失败。改为包级哨兵 ErrKeyNotFound,以 errors.Is 判别(对照 Pebble 的 base.ErrNotFound、Badger 的 ErrKeyNotFound)。

同时按 Go 的错误文本约定(小写开头、无尾随标点、包名前缀)重写四处违规:

"NO DATA IN MEM" / "NO DATA IN MEMTABLE" ErrMemTableUnavailable
"Key not found" ErrKeyNotFound
"dont keep"(原文无法辨义) ErrNoEntries

新增 errors_test.go 固定契约:不存在的 key 与删除墓碑均须满足 errors.Is(err, ErrKeyNotFound)

边界说明:本 PR 只让错误在进程内可判别;BanNet 线协议仍把「不存在」与「内部错误」回同一状态码,是否新增状态属协议决策,留到 SDK 阶段处理(SDK 恰恰需要这个区分)。

2. 接口去 I 前缀,并删除空抽象

I 前缀是 C#/Java 惯例。原 10 个接口的名字又与其实现同名,去掉 I 即冲突,故按行为重新命名:

依据
IConnect Conn 对照 net.Conn(接口)/ net.TCPConn(实现)
IRouter Handler 对照 http.Handler
IMessage Frame 协议注释本就称「帧」;Message 名不可让出(见下)
IDataPack Codec Pack/UnPack 即编解码
IMsgHandle Dispatcher 把帧分派给 Handler
IConnManager ConnRegistry 连接注册表
IRequest Request(实现降为包内 request 接口取短名

Message 结构体保留原名:cmd/ban-benchcmd/ban-cli 都以 *bannet.Message 做类型断言,且 pkg/utils.Message 是该接口的第二个实现——故接口取 Frame

删除三个空抽象(Go Code Review Comments:不要在需要之前定义接口):

  • IServer — 单一实现、无测试替身。它此前反而制造摩擦:NewServer 返回接口,集成测试为拿到具体字段不得不写 NewServer().(*bannet.Server) 断言。现返回 *Server(accept interfaces, return structs),断言消失。
  • ISSTable — 除自身 var _ 断言外零消费者
  • IMemTable — 仅作为 Engine.memTable 字段类型,实现唯一,且扁平化后与实现同包,不提供跨包解耦。同类项目亦持具体类型(Badger 持 *skl.Skiplist,Pebble 持 *memTable)。

3. 其余命名

  • MataMeta:metadata 缩写被一致误拼 38 处;同文件内 EnsureMeta 却拼对,连自身都不一致。切片及其访问器同时改复数(metas / Metas)。
  • 去 19 个 getter 的 Get 前缀(Effective Go 明确反对)。三处新名与既有字段冲突,改用更贴切的名字:GetDataPayloadMessage.Data 已占用)、GetConnMgrConnsGetConnIDIDGetApplyCh直接删除——raft.Raft.ApplyCh 本就是导出字段,为导出字段包 getter 是多余的(去前缀后同名即冲突,恰好暴露了这点)。生成代码里的 GetKey/GetValue/GetSuccess 一律不动。
  • Command.Type 改具名类型 + 常量:原先八个调用点各自重复 "Put"/"Delete" 字面量。诚实说明其边界:这不是编译期强校验,无类型字符串常量可隐式转为 CommandTypeType: "Pt" 仍能编译;强校验需 int 枚举,但 Commandjson.Marshal 写入 Raft 日志,改数值表示会破坏既有日志兼容性,故保留字符串底层类型。
  • Ariprouters:原名无法辨义,实质是 msgID → Handler 的路由表。
  • test_grpckvgrpc:下划线不合包名惯例,且 test_ 前缀会让人误判为测试代码。仅改 Go 侧;protobuf 包名保持不变(无 protoc-gen-go 无法重新生成,且该名字编入 rawDesc 并决定 gRPC 方法路径)。踩坑记录见该 commit:首次全局替换破坏了 rawDesc 的长度前缀,protobuf 运行时 init 直接 panic。

未纳入

service/shardkv/command.goOp string(带 json tag)是另一套独立命令类型,是否具名化属其自身设计问题,未混入。

验证

go build ./...(含 -tags pprof)、go vet ./...go test -race ./... 全绿。

🤖 Generated with Claude Code

NeverENG and others added 7 commits August 13, 2026 00:21
metadata 的缩写被一致地误拼为 Mata,覆盖类型 SSTableMata、字段 mata、方法 AddMata /
RemoveMata / GetAllMata / publishMata 及相关注释共 38 处。同一文件内 EnsureMeta 却拼写
正确,故该误拼连自身都不一致。

同时把切片字段与其访问器改为复数:mata → metas、GetAllMata → GetAllMetas,使名称与其
返回多个元素的语义相符(Get 前缀留待后续提交统一去除)。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
此前「key 不存在」在 memtable.go 四处各自 errors.New 一个新对象,调用方因此无法把它
与读盘失败等真实故障区分开——两者在上层看来都只是「某个 error」,只能一视同仁地当作
失败处理。改为包级哨兵 ErrKeyNotFound,调用方以 errors.Is 判别(对照 Pebble 的
base.ErrNotFound、Badger 的 ErrKeyNotFound)。

同时按 Go 的错误文本约定(小写开头、无尾随标点、以包名前缀标明来源)重写四处违规文本:
  "NO DATA IN MEM" / "NO DATA IN MEMTABLE" → ErrMemTableUnavailable
  "Key not found"                          → ErrKeyNotFound
  "dont keep"(原文即无法辨义)            → ErrNoEntries
service 层 KVServer.Get 亦返回 storage.ErrKeyNotFound 而非自建错误。

新增 errors_test.go 固定该契约:不存在的 key 与删除墓碑均须满足 errors.Is(err,
ErrKeyNotFound),空条目集落盘须为 ErrNoEntries。

注:本提交只让错误在进程内可判别;BanNet 线协议仍把「不存在」与「内部错误」回同一个
状态,是否新增状态码属协议决策,留待 SDK 阶段一并处理。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I 前缀是 C#/Java 惯例,Go 不使用——接口应按行为命名(Effective Go:单方法接口以 -er
结尾;对照 etcd 的 Storage/Node、Pebble 的 Reader/Writer)。原 8 个接口的名字又与其实现
同名(IServer/Server、IRequest/Request…),去掉 I 即冲突,故按行为重新命名:

  IConnect     → Conn         对照 net.Conn(接口)/ net.TCPConn(实现);此处实现为 Connection
  IRouter      → Handler      对照 http.Handler;方法即 PreHandle/Handle/PostHandle
  IMessage     → Frame        协议注释本就称「帧」;Message 名不可让出(见下)
  IDataPack    → Codec        Pack/UnPack 即编解码
  IMsgHandle   → Dispatcher   把帧分派给 Handler
  IConnManager → ConnRegistry 连接注册表
  IRequest     → Request      实现降为包内私有 request(仅 connection.go 构造)

Message 结构体保留原名:cmd/ban-bench 与 cmd/ban-cli 都以 *bannet.Message 做类型断言,
且 pkg/utils.Message 是该接口的第二个实现——故接口取 Frame 而非 Message。

IServer 直接删除而非改名:它只有一个实现、无测试替身,Go 的惯例是此时不应存在接口
(Go Code Review Comments:不要在需要之前定义接口)。其唯一代价此前反而是摩擦——
NewServer 返回接口,集成测试为拿到具体字段不得不写 NewServer().(*bannet.Server) 断言。
现 NewServer 返回 *Server(accept interfaces, return structs),该断言随之消失。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
两者各只有一个实现、无测试替身,且扁平化后与实现同包,接口不提供任何跨包解耦:
- ISSTable 除自身的 var _ 断言外零消费者,是完全无人使用的抽象
- IMemTable 仅作为 Engine.memTable 的字段类型,实现唯一

Go Code Review Comments 明确「不要在需要之前定义接口」;同类项目亦持具体类型
(Badger 持 *skl.Skiplist、Pebble 持 *memTable)。Engine 改为直接持 *MemTable,
interfaces.go 随之删除。将来确有第二个实现或需要测试替身时再引入接口即可。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Effective Go 原文:it's neither idiomatic nor necessary to put Get into the getter's
name。setter 保留 Set 前缀(同一约定),故仅改 getter。

  Metas ApplyCh CommitIndex ID Conns Conn Payload FSM HeadLen LastCheck
  LevelFiles Log MsgData MsgID MsgLen Property Raft State TCPConn

三处新名与既有字段冲突,改用更贴切且不冲突的名字而非机械去前缀:
  GetData    → Payload(Message.Data 字段已占用 Data;Payload 也更准确地表达「帧负载」)
  GetConnMgr → Conns  (Server.ConnMgr 字段已占用)
  GetConnID  → ID     (Connection.ConnID 字段已占用;ID 亦不与类型名 Conn 叠字)

GetApplyCh 则直接删除:raft.Raft 的 ApplyCh 本就是导出字段,为导出字段再包一层 getter
在 Go 中是多余的(去前缀后二者同名即编译冲突,恰好暴露了这一点)。两处调用方改为直接
读该字段。

生成代码 (*.pb.go) 中的 GetKey / GetValue / GetSuccess 一律不动:那由 protoc 生成,
改名会在下次重新生成时丢失,且属 gRPC 侧契约。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
原先 Type 为裸 string,八个调用点各自重复 "Put"/"Delete" 字面量,switch 亦与字面量
比较:拼错一个字母不报错也不告警,命令会静默地既不写入也不删除(switch 落空)。改为
CommandType + CommandPut/CommandDelete,拼写只存在一份。

诚实说明其边界:这不是编译期强校验——无类型字符串常量可隐式转换为 CommandType,
Type: "Pt" 仍能通过编译。要做到强校验需改为 int 枚举,但 Command 经 json.Marshal
写入 Raft 日志,改变数值表示会破坏既有日志兼容性,故保留字符串底层类型。

顺带把 bannet.MsgHandle 的字段 Arip 改名为 routers:原名无法辨义,其实质是
msgID → Handler 的路由表。

未纳入:service/shardkv/command.go 的 Op string 是另一套独立的命令类型(含 json tag),
应否一并具名化属其自身的设计问题,不在本次改动范围。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
包名有两处问题:下划线不合 Go 包名惯例(要求短的全小写单词),且 test_ 前缀会让人误判
为测试专用代码——它实际是生产用的 gRPC 服务实现与客户端。

仅改 Go 侧(目录、package 子句、go_package 选项)。protobuf 包名 test_grpc 保持不变,
因为:一是本机无 protoc-gen-go 无法重新生成;二是该名字被编入生成文件的 rawDesc(长度
前缀的序列化描述符)并决定 gRPC 方法路径 /test_grpc.KVService/Put,不可手工改写。
proto 包名与 Go 包名不同在 protobuf 生态中属常见情形。

过程记录:首次尝试时对 *.pb.go 做了全局 test_grpc → kvgrpc 替换,破坏了 rawDesc 中的
长度前缀(9 字节缩为 6 字节),protobuf 运行时在 init 阶段直接 panic。改为只替换第 7 行
的 package 子句。go_package 选项已同步更新,故将来重新生成会得到同样的 package kvgrpc。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

🐯 BanGD 数据库内核评审

整体风险:🟢 低

变更总结:本 PR 是承接 #250标识符命名重构批,核心分为三块:

  1. 错误契约修复(真正语义改动):把「key 不存在」在 memtable.go 四处各自 errors.New 的临时错误统一为存储层包级哨兵 ErrKeyNotFound(对照 Pebble base.ErrNotFound、Badger ErrKeyNotFound),并新增 ErrMemTableUnavailableErrNoEntries,使调用方得以用 errors.Is 把「无此键」(正常查询结果)与「读盘失败」「引擎异常」等真实故障判别开。新增 errors_test.go 固定该契约。作者诚实声明边界:BanNet 线协议仍不区分「不存在」与「内部错误」,留待 SDK 阶段。同时 Command.Type 由裸 string 改为具名类型 CommandType + 常量(保留了字符串底层类型以维持 Raft 日志 JSON 序列化兼容,不追求编译期强校验)。

  2. 接口去 I 前缀 + 删除空抽象:10 个接口按行为重命名(IConnect→ConnIRouter→HandlerIMessage→FrameIDataPack→CodecIMsgHandle→Dispatcher 等),实现与接口同名冲突的降级为包内 request。删除 IServer(单一实现,NewServer 改返回 *Server,消除集成测试的类型断言)、ISSTable(零消费者)、IMemTable(唯一实现、与实现同包不提供解耦)。

  3. 其余命名整理Mata→Meta(38 处修正拼写)、去 19 个 getter 的 Get 前缀、删冗余的 GetApplyChArip→routerstest_grpc→kvgrpc 目录改名。gRPC 侧 protobuf 包名保持 test_grpc 不变(无法重新生成,且编入 rawDesc)。

总体上是一次覆盖 bannet/raft/storage/service/kvgrpc 五个模块的大规模重命名,但语义改动很少(主要是错误契约),无并发/存储/崩溃安全相关结构变动。

本评审不阻塞合入;架构级建议以 Issue 形式跟踪,普通问题在下方内联列出。

架构问题(共 3 项)

普通问题(共 3 项)

⚠️ [重要 · 错误处理] storage/memtable.go:144 Get 路径上 SSTable 墓碑与内存未命中返回值语义被统一

  • diff 截断处 getFromSSTables 返回 (nil, true) 表示 hit 但 value 为 nil(SSTable 墓碑),Get 将其转为 ErrKeyNotFound;而 dirty.search/active.search 的未命中(found==false)不直接返回,会继续下穿查找。这两条路径在语义上是等价的(都是『key 不存在或墓碑』),本 PR 把它们统一为同一哨兵是正确的。但需注意:ScaneRange 的墓碑跳过逻辑在 Get 中并无对应——Get 只是基于单个墓碑返回 ErrKeyNotFound,不存在不一致。此条实无需改动,仅提示调用 errors.Is(err, ErrKeyNotFound) 即可安全处理三种墓碑来源(active/dirty/SSTable)。
  • 建议:无需改动。仅确认调用方统一用 errors.Is 而非比较错误文本即可。

⚠️ [重要 · 逻辑错误] service/fsm.go:132 applyStandalone 的 switch 在未知 Type 下静默落空

  • applyStandalone 的 switch 现将 default 分支完全没有——这在此前裸 string 版本、以及本 PR 具名类型版本中都存在。若某处构造了未知 Type(例如未来新增命令类型但遗漏本 switch),该命令会静默地既无 WAL append 也无 memtable 写入,被当作成功吞掉(Write 返回 nil)。这与本 PR 在 CommandType 注释里承认的『静默地既不写入也不删除』风险一致,但架构级建议里我让加了 default 报错。此处作为普通发现标注:至少应在 default 分支返回明确错误。
  • 建议:在 applyStandaloneApply 的 switch 各加 default 分支返回/记录 unknown command type %q,把静默落空变为显式。

💡 [建议 · 错误处理] storage/errors.go:13 ErrNoEntries 与 FlushToSSTable 的空条目提前返回存在双路径

  • memtable.FlushToSSTable 对空 entries 提前 return nil(不触发 WriteToSSTable,故不返回 ErrNoEntries);而 SSTable.WriteToSSTable(nil) 直接返回 ErrNoEntries。两条路径对『空集』的契约不一致:直接调 WriteToSSTable(nil) 拿 ErrNoEntries,经 FlushToSSTable(nil) 却拿 nil。若调用方基于 ErrNoEntries 做『本次无写入』逻辑(如 metrics),可能在不同入口下计数不一致。
  • 建议:统一契约:或让 FlushToSSTable 的空集也返回 ErrNoEntries(但从语义上 FlushToSSTable 返回 nil 表示『无操作、非错误』更合理),或让 WriteToSSTable(nil) 也返回 nil。建议保留 WriteToSSTable 的 ErrNoEntries(作为断言错误),但明确 FlushToSSTable 是『无操作的正常路径』,文档化即可。测试已覆盖 ErrNoEntries 的 WriteToSSTable 路径。

本次评审消耗 token:共 303470 tokens(输入 289126,输出 5512,缓存命中 8832,缓存写入 0)|维度 [concurrency, memory, lock, storage, schema]|补充阅读周边文件 [storage/memtable.go, storage/sstable.go, storage/engine.go]|对抗式复核 3 票/条,过滤疑似误报 0 条

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant