feat(docker): add grpc-health-probe for health checks and update vers… #2581
Conversation
WalkthroughVersion bumped from v1.4.8 to v1.4.9 across metadata and API documentation. Dockerfile configured to build and install grpc-health-probe binary. Health check updated to use grpc_health_probe targeting port 3478. Comprehensive test suite added for singleflight tenant-isolation behavior. Code comments added for clarity, and debug logging removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Dockerfile.local (1)
6-12: Consider pinning grpc-health-probe to a specific version.The Dockerfile clones grpc-health-probe from the main branch without specifying a version or commit SHA. This could lead to non-reproducible builds if the upstream repository changes.
Consider pinning to a specific version or commit:
# Build grpc-health-probe WORKDIR /tmp -RUN git clone https://github.com/grpc-ecosystem/grpc-health-probe.git && \ +RUN git clone --depth 1 --branch v0.4.35 https://github.com/grpc-ecosystem/grpc-health-probe.git && \ cd grpc-health-probe && \ CGO_ENABLED=0 go install -a -tags netgo -ldflags=-w && \ cp /go/bin/grpc-health-probe /usr/local/bin/grpc_health_probe && \ cd / && rm -rf /tmp/grpc-health-probeThis ensures reproducible builds and prevents unexpected breakage from upstream changes.
internal/storage/decorators/singleflight/data_reader_test.go (1)
43-43: Consider making test delays configurable or environment-aware.The hardcoded 10ms sleep simulates work but could cause flaky tests in slower CI environments or under load. While this is acceptable for local development tests, consider if the delay is necessary or if there are alternative ways to test the deduplication behavior.
If timing becomes an issue, you could use channels or other synchronization primitives instead of sleep:
func (m *MockDataReader) HeadSnapshot(ctx context.Context, tenantID string) (token.SnapToken, error) { // Track call count per tenant m.mu.Lock() counter, exists := m.headSnapshotCalls[tenantID] if !exists { counter = new(int64) m.headSnapshotCalls[tenantID] = counter } m.mu.Unlock() // Increment call count atomic.AddInt64(counter, 1) - // Simulate some work - time.Sleep(10 * time.Millisecond) + // Brief yield to allow concurrent goroutines to pile up + runtime.Gosched() return token.NoopToken{Value: "snapshot-" + tenantID}, nil }internal/storage/decorators/singleflight/schema_reader_test.go (1)
42-42: Same timing consideration as in data_reader_test.go.The 10ms sleep could potentially cause flaky tests in slower environments. See the comment in data_reader_test.go for suggested alternatives if this becomes an issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/pb/base/v1/openapi.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
Dockerfile.local(1 hunks)docker-compose.yaml(1 hunks)docs/api-reference/apidocs.swagger.json(1 hunks)docs/api-reference/openapiv2/apidocs.swagger.json(1 hunks)internal/info.go(1 hunks)internal/storage/decorators/singleflight/data_reader.go(1 hunks)internal/storage/decorators/singleflight/data_reader_test.go(1 hunks)internal/storage/decorators/singleflight/schema_reader.go(1 hunks)internal/storage/decorators/singleflight/schema_reader_test.go(1 hunks)internal/storage/decorators/singleflight/singleflight_test.go(1 hunks)internal/storage/postgres/utils/common.go(0 hunks)proto/base/v1/openapi.proto(1 hunks)
💤 Files with no reviewable changes (1)
- internal/storage/postgres/utils/common.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/storage/decorators/singleflight/data_reader_test.go (2)
internal/storage/storage.go (1)
NoopDataReader(42-42)internal/storage/decorators/singleflight/data_reader.go (2)
DataReader(15-18)NewDataReader(21-23)
internal/storage/decorators/singleflight/schema_reader_test.go (2)
internal/storage/storage.go (1)
NoopSchemaReader(124-124)internal/storage/decorators/singleflight/schema_reader.go (2)
SchemaReader(14-17)NewSchemaReader(20-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Test with Coverage
- GitHub Check: Test with Coverage
🔇 Additional comments (12)
internal/storage/decorators/singleflight/schema_reader.go (1)
46-46: LGTM! Good clarification.The inline comment clearly explains the tenant isolation mechanism in the singleflight deduplication pattern, improving code maintainability.
docs/api-reference/apidocs.swagger.json (1)
6-6: Version bump looks good.The API documentation version is correctly updated to v1.4.9, consistent with the version changes in other files.
internal/info.go (1)
26-26: Version constant updated correctly.The primary version identifier is now v1.4.9, which will be reflected in the application banner and aligns with the version metadata in API documentation.
proto/base/v1/openapi.proto (1)
12-12: OpenAPI version metadata updated consistently.The proto annotation version matches the application version (v1.4.9), ensuring consistency between the codebase and generated API documentation.
Dockerfile.local (1)
4-4: Git dependency added for grpc-health-probe build.The addition of
gitis necessary for cloning the grpc-health-probe repository. This is a minimal change for the build process.internal/storage/decorators/singleflight/data_reader.go (1)
57-57: Good documentation of tenant isolation behavior.The added comment clarifies that
tenantIDis used as the deduplication key, ensuring proper tenant isolation. This is important context for understanding the singleflight behavior in a multi-tenant system.internal/storage/decorators/singleflight/singleflight_test.go (1)
1-13: Clean test suite setup.The test file correctly sets up a Ginkgo test suite for the singleflight package. The structure is standard and appropriate.
internal/storage/decorators/singleflight/data_reader_test.go (2)
16-46: Well-designed mock with proper concurrency handling.The MockDataReader correctly uses:
- Per-tenant call tracking with a mutex-protected map
- Atomic operations for thread-safe counter increments
- Embedded NoopDataReader for default implementations
57-181: Comprehensive test coverage for singleflight behavior.The test suite effectively validates:
- Deduplication of concurrent requests for the same tenant
- Proper isolation between different tenants
- Correct results returned to each tenant
- No deduplication for sequential requests
This provides strong confidence in the singleflight decorator's correctness.
internal/storage/decorators/singleflight/schema_reader_test.go (2)
15-45: Consistent mock implementation pattern.The MockSchemaReader follows the same well-designed pattern as MockDataReader, with proper concurrency handling using mutexes and atomic operations. This consistency makes the test suite easier to understand and maintain.
56-180: Thorough test coverage for SchemaReader singleflight behavior.The test suite validates the same critical behaviors as the DataReader tests:
- Concurrent request deduplication per tenant
- Cross-tenant isolation
- Correct version strings per tenant
- Sequential request handling
The parallel test structure across both reader types provides confidence in the singleflight decorator's general correctness.
docker-compose.yaml (1)
15-15: No issues found. The healthcheck configuration is correct.The verification confirms:
- Port 3478 is the correct and consistent gRPC service port throughout the codebase (default address configured in
pkg/grpcserver/server.go)- The
-addrflag format is valid; grpc_health_probe accepts both space-separated (-addr localhost:3478) and equals-separated (-addr=127.0.0.1:3478) syntax- The grpc-health-probe binary is properly installed from the grpc-ecosystem repository in Dockerfile.local
- The distinction between port 3476 (HTTP/REST API) and 3478 (gRPC) is maintained correctly throughout the codebase
…ion to v1.4.9
Summary by CodeRabbit
Chores
Tests