Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Simple-Cache’s public-facing docs and refactors several internals (cache expiration, gRPC/REST API surface, Raft transport/node lifecycle, and process shutdown behavior).
Changes:
- Add comprehensive
README.mdand update API docs/Swagger artifacts. - Extend ExpireKey API to accept an
expireduration and refactor cache expiration tracking to use a heap + index for faster deletions. - Introduce graceful shutdown and lifecycle management for Raft/metrics/HTTP servers, plus additional Raft request fields and synchronization.
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | New/expanded project documentation, architecture, API examples |
| pkg/server/server.go | Wire ExpireKeyRequest.expire into command execution |
| pkg/raft/types.go | Extend AppendEntries with PrevLogIndex/PrevLogTerm |
| pkg/raft/node.go | Add mutex/WaitGroup/Close, update election/submit/append logic |
| pkg/raft/http_transport.go | Add shared HTTP client, peer locking, graceful server shutdown |
| pkg/proto/expire_key.proto | Add expire field to ExpireKeyRequest |
| pkg/pb/expire_key.pb.go | Regenerated protobuf Go code for ExpireKeyRequest |
| pkg/pb/cache.pb.gw.go | Regenerated/modified grpc-gateway bindings for ExpireKey |
| pkg/metrics/metrics.go | Make metrics init idempotent; add stop channel + Close() |
| pkg/config/config.go | Add watcher logger hooks and hot-reload logging |
| pkg/command/command.go | Add basic key/pattern validation; change ExpireKey command payload |
| pkg/cmd/swagger/simple_cache.swagger.json | Update Swagger to reflect ExpireKey query param |
| pkg/cmd/main.go | Improve config load error handling; add graceful shutdown sequence |
| pkg/client/client.go | Update client ExpireKey signature to accept ttl |
| pkg/client/client_test.go | Adjust tests for new ExpireKey signature/pattern |
| pkg/cache/set.go | Improve Set cleanup for stale expirations; reduce log verbosity |
| pkg/cache/search.go | Reduce log verbosity; simplify search implementation |
| pkg/cache/reset.go | Reset now reinitializes radix tree + expiration heap/index |
| pkg/cache/heap.go | New heap implementation with index tracking support |
| pkg/cache/get.go | Use delInternal for expired-key cleanup |
| pkg/cache/expiration.go | Implement SetExpiration operation using heap/index |
| pkg/cache/del.go | Optimize expiration entry deletion via index map |
| pkg/cache/cleanup.go | Update cleanup to use heap Peek/index and refresh key metrics |
| pkg/cache/cache.go | Add expirationIndex + heap swap callback wiring |
| pkg/cache/cache_test.go | Update tests for new heap implementation |
| .gitignore | Ignore .trae and adjust config.yaml ignore line |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Normalize: if no IP specified, use localhost | ||
| if _, _, err := net.SplitHostPort(peerHost); err != nil { | ||
| peerHost = "localhost" + peerHost | ||
| } | ||
| return peerPort == selfPort | ||
| if _, _, err := net.SplitHostPort(selfHost); err != nil { | ||
| selfHost = "localhost" + selfHost | ||
| } | ||
|
|
||
| return peerHost == selfHost |
There was a problem hiding this comment.
isSelf compares the full host:port string, which fails when the node listens on ":9090" but peers are configured as "http://127.0.0.1:9090" (common in the provided configs). That makes the node send AppendEntries/Vote RPCs to itself and can inflate ack/vote counts. Consider normalizing to compare ports (or resolve/normalize IPs) so ":9090" and "127.0.0.1:9090" are treated as self.
| if n.Role() == Leader { | ||
| continue | ||
| } | ||
| if time.Now().After(n.electionDeadline) { |
There was a problem hiding this comment.
electionDeadline is read in electionLoop without holding n.mu, but it’s written by resetElectionDeadline() (called from multiple goroutines, including under n.mu in RPC handlers). This is a data race under -race. Protect all reads/writes of electionDeadline with the same mutex (or store it atomically).
| if time.Now().After(n.electionDeadline) { | |
| n.mu.Lock() | |
| electionDeadline := n.electionDeadline | |
| n.mu.Unlock() | |
| if time.Now().After(electionDeadline) { |
| // Append new entries | ||
| for _, e := range req.Entries { | ||
| _ = n.storage.Append(e) | ||
| n.lastLogIndex++ | ||
| n.lastLogTerm = req.Term | ||
| } |
There was a problem hiding this comment.
onAppendEntries appends incoming log entries to storage but never applies them to the state machine (n.applier). The comment says entries are applied “as they arrive via the Entries field”, but there is no call that decodes/applies req.Entries, so followers will not advance their FSM state and a leader change will lose committed state. Either apply entries as they arrive (decode to a concrete command type) or implement log read+apply for indices up to commitIdx.
| var ( | ||
| filter_CacheService_ExpireKey_0 = &utilities.DoubleArray{Encoding: map[string]int{"key": 0}, Base: []int{1, 1, 0}, Check: []int{0, 1, 2}} | ||
| ) |
There was a problem hiding this comment.
The generated query-parameter filter for ExpireKey only contains key, so runtime.PopulateQueryParameters will never populate the new expire field. As a result, REST calls to /v1/{key}/expire can’t set expiration and will hit the server-side “expire duration must not be empty” error. Regenerate/fix the gateway bindings so the filter includes expire (or change the HTTP annotation to accept a body).
| curl -X POST http://localhost:8080/v1/greeting/expire \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"expire": "30s"}' |
There was a problem hiding this comment.
The REST Expire example sends expire in a JSON body, but the proto HTTP annotation for ExpireKey doesn’t specify body: "*", and the generated gateway handler only populates query parameters. Either update the API to accept a JSON body for ExpireKey or change this example to pass expire as a query parameter.
| curl -X POST http://localhost:8080/v1/greeting/expire \ | |
| -H "Content-Type: application/json" \ | |
| -d '{"expire": "30s"}' | |
| curl -X POST "http://localhost:8080/v1/greeting/expire?expire=30s" |
| // Set new expiration | ||
| item.expiration = time.Now().Add(duration) | ||
| heap.Push(c.expirationHeap, &expirationEntry{ | ||
| key: key, | ||
| expiration: item.expiration, | ||
| }) |
There was a problem hiding this comment.
Current semantics don’t support “remove expiration” (persistent key). If expire is empty or parses to a non-positive duration, SetExpiration should clear item.expiration and ensure any existing heap entry is removed, rather than setting expiration = time.Now().Add(duration) and pushing a new heap entry (which can immediately expire for 0s). This also needs to line up with the client’s ttl<=0 behavior.
| // If ttl <= 0, the expiration is removed (key becomes persistent). | ||
| func (c *Client) ExpireKey(ctx context.Context, key string, ttl time.Duration) (bool, error) { |
There was a problem hiding this comment.
ExpireKey is documented as “ttl <= 0 removes expiration”, but formatTTL returns an empty string for ttl <= 0, and the server-side ExpireKeyCommand currently treats empty Expire as an error. Either change formatTTL to send an explicit value (e.g., "0s") and handle it server-side as “clear TTL”, or update the client docs/semantics so callers aren’t promised unsupported behavior.
| // If ttl <= 0, the expiration is removed (key becomes persistent). | |
| func (c *Client) ExpireKey(ctx context.Context, key string, ttl time.Duration) (bool, error) { | |
| // ttl must be greater than 0. Removing an existing expiration is not supported | |
| // by this client/server API contract. | |
| func (c *Client) ExpireKey(ctx context.Context, key string, ttl time.Duration) (bool, error) { | |
| if ttl <= 0 { | |
| return false, errors.New("ttl must be greater than 0 for ExpireKey") | |
| } |
| if c.Expire == "" { | ||
| return &pb.ExpireKeyResponse{Success: false, Existed: false}, fmt.Errorf("expire duration must not be empty") | ||
| } | ||
| existed := cache.SetExpiration(c.Key, c.Expire) | ||
| return &pb.ExpireKeyResponse{Success: true, Existed: existed}, nil |
There was a problem hiding this comment.
ExpireKeyCommand currently rejects Expire == "", but the client helper formatTTL produces an empty string for ttl <= 0 and the README claims ttl=0 removes expiration. Also, cache.SetExpiration can fail to parse a duration and simply returns false, yet this command returns Success: true in that case. Consider: (1) defining a clear “clear TTL” representation (e.g., empty/"0s") and supporting it, and (2) propagating duration-parse errors so invalid TTL doesn’t look like a missing key.
| 1. **每个节点需要独立的端口** — gRPC、HTTP、Raft、Metrics 各自使用不同端口 | ||
| 2. **所有节点的 `peers` 列表必须一致** — 包含集群中所有节点的 Raft HTTP 地址 | ||
| 3. **建议至少 3 个节点** — Raft 需要多数派确认，2 个节点无法容忍任何故障 | ||
| 4. **客户端可连接任意节点** — 写入请求会自动转发到 Leader，读取请求在本地执行 |
There was a problem hiding this comment.
This claim says reads are executed locally on any node, but the current gRPC CacheService.Get implementation rejects requests when the node role is not Leader (returns "not leader"). Either update the docs to state reads must go to the leader, or change the server behavior to allow follower reads if that’s the intended design.
| 4. **客户端可连接任意节点** — 写入请求会自动转发到 Leader，读取请求在本地执行 | |
| 4. **客户端可连接任意节点** — 写入请求会自动转发到 Leader；当前读取请求也需要由 Leader 处理，非 Leader 节点上的 gRPC `Get` 请求会返回 `not leader` |
| go grpcServer.Serve(lis) | ||
|
|
||
| sig := <-sigCh | ||
| logger.Info("received shutdown signal", zap.String("signal", sig.String())) |
There was a problem hiding this comment.
grpcServer.Serve(lis) is started in a goroutine and its returned error is ignored. If Serve exits early (e.g., listener closed unexpectedly), main will block forever waiting for a signal. Consider capturing the Serve error (e.g., via an errgroup/channel) and triggering shutdown/logging when it returns.
| go grpcServer.Serve(lis) | |
| sig := <-sigCh | |
| logger.Info("received shutdown signal", zap.String("signal", sig.String())) | |
| serveErrCh := make(chan error, 1) | |
| go func() { | |
| if err := grpcServer.Serve(lis); err != nil && !errors.Is(err, grpc.ErrServerStopped) { | |
| serveErrCh <- err | |
| } | |
| close(serveErrCh) | |
| }() | |
| select { | |
| case sig := <-sigCh: | |
| logger.Info("received shutdown signal", zap.String("signal", sig.String())) | |
| case err := <-serveErrCh: | |
| if err != nil { | |
| logger.Error("gRPC server exited unexpectedly", zap.Error(err)) | |
| } | |
| } |
No description provided.