Made common connection to the storage adapter to omit huge memory consumption #7
Conversation
WalkthroughThe changes update the application’s architecture by integrating an external storage connection into the Changes
Sequence Diagram(s)sequenceDiagram
participant S as Serve
participant B as Bootstrap
participant C as Cache
participant ST as Storage
participant A as Application
S->>B: NewApplication(ctx, config)
B->>C: newCache(cfg) (if caching enabled)
B->>ST: newStorageConnection(cfg)
B->>A: Application.New(ctx, config, storage, cache)
A-->>B: Application instance
B-->>S: Application instance
sequenceDiagram
participant Client as Client
participant Server as Server
participant Handler as LeaseHandler
participant Storage as Storage
Client->>Server: HTTP Request (/lease or /keepalive)
Server->>Handler: Call CreateLease/ReviveLease(ctx, storage, ...)
Handler->>Storage: Process request using storage connection
Storage-->>Handler: Return result
Handler-->>Server: Response generated
Server-->>Client: HTTP Response
Possibly related PRs
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code Graph Analysis (2)internal/delivery/http/server.go (4)
internal/application/application.go (4)
🔇 Additional comments (10)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/bootstrap/bootstrap.go (1)
30-38: Clean cache initialization handling.The function correctly handles both enabled and disabled cache scenarios, with appropriate logging. However, consider enhancing error handling by checking for any potential initialization errors when creating the cache.
func newCache(cfg *config.Config) (*cache.Cache, error) { if cfg.Cache.Enabled { log.Info("Cache is enabled") - return cache.New(cfg.Cache.Size), nil + cache := cache.New(cfg.Cache.Size) + if cache == nil { + return nil, fmt.Errorf("failed to initialize cache") + } + return cache, nil } log.Info("Cache is disabled") return nil, nil }internal/application/application_test.go (1)
89-271: Consider reusing a single mock storage instance across test casesCurrently, each test function creates its own storage connection instance. Since the mock implementation has no state, you could potentially define a single instance at the package level to reduce redundancy.
package application import ( // existing imports... "github.com/tentens-tech/shared-lock/internal/infrastructure/storage/mock" ) // Add a single mock storage instance at package level +var sharedMockStorage = &mock.Storage{} func createTestConfig() *config.Config { // existing code... } func TestGetLeaseHandler(t *testing.T) { // ... for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // ... - storageConnection := &mock.Storage{} + storageConnection := sharedMockStorage // ... }) } } // Apply similar changes to other test functions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/application/application.go(2 hunks)internal/application/application_test.go(10 hunks)internal/bootstrap/bootstrap.go(1 hunks)internal/delivery/http/server.go(3 hunks)internal/delivery/serve.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal/delivery/serve.go (1)
internal/bootstrap/bootstrap.go (1)
NewApplication(40-54)
internal/delivery/http/server.go (2)
internal/application/application.go (2)
GetLeaseHandler(45-123)KeepaliveHandler(125-156)internal/config/config.go (1)
Config(28-33)
internal/application/application_test.go (2)
internal/infrastructure/storage/mock/mock.go (1)
Storage(9-9)internal/application/application.go (2)
GetLeaseHandler(45-123)KeepaliveHandler(125-156)
🔇 Additional comments (18)
internal/bootstrap/bootstrap.go (2)
16-28: Well-structured storage connection factory function.This new function effectively handles different storage types (etcd or mock) based on the configuration. Error handling is appropriate, and the code is clean and maintainable.
40-54: Good application initialization with proper error handling.The function correctly orchestrates the initialization of components and handles errors appropriately. The approach of externalizing the bootstrap logic makes the codebase more maintainable and follows the Separation of Concerns principle.
internal/application/application.go (4)
29-34: Good addition of StorageConnection field to the Application struct.Adding the StorageConnection field to the Application struct allows for a single shared storage connection, which should help reduce memory consumption as mentioned in the PR title.
36-43: Constructor updated correctly to include storage connection.The New function signature has been properly updated to accept the storage connection as a parameter, and the struct initialization has been adjusted accordingly.
45-123: GetLeaseHandler now correctly accepts the storage connection.The handler has been updated to receive the storage connection as a parameter instead of creating it internally. This change is consistent with the PR's goal of reducing memory consumption by reusing a single storage connection.
125-156: KeepaliveHandler now correctly accepts the storage connection.Similar to GetLeaseHandler, this function has been updated to receive the storage connection as a parameter, maintaining consistency throughout the codebase.
internal/delivery/http/server.go (4)
6-7: Added pprof support for performance profiling.Good addition of the pprof package import, which will enable runtime profiling capabilities. This is particularly useful for identifying potential memory issues.
30-31: Added pprof HTTP endpoint.This correctly adds a handler for the pprof debugging endpoint, which is essential for runtime profiling. Ensure this endpoint is properly secured in production environments.
When deployed to production, ensure that the pprof endpoint is not publicly accessible, as it could expose sensitive information. Consider adding authentication or IP-based restrictions for this endpoint.
43-45: handleLease now uses the storage connection from the app.The handler now correctly passes the application's storage connection to the GetLeaseHandler function, implementing the shared connection approach.
47-49: handleKeepalive now uses the storage connection from the app.The handler now correctly passes the application's storage connection to the KeepaliveHandler function, implementing the shared connection approach.
internal/delivery/serve.go (2)
11-12: Updated import to use the new bootstrap package.Correctly replaced the application import with the bootstrap import to leverage the new initialization logic.
40-44: Improved application initialization with proper error handling.The code now uses the bootstrap package to create the application instance, which includes initializing both the cache and storage connection. The error handling has been improved to catch initialization failures.
internal/application/application_test.go (6)
19-19: New import correctly added for mock storageThe addition of the mock package import is necessary to support the refactored architecture that uses an external storage connection.
89-90: Storage connection correctly integrated in TestGetLeaseHandlerThe mock storage connection is properly created and passed to the GetLeaseHandler function, aligning with the new signature that takes an explicit storage connection. This change supports the PR objective of reusing storage connections to reduce memory consumption.
Also applies to: 111-111
126-127: Storage connection correctly integrated in TestGetLeaseHandlerConcurrentThe mock storage instance is properly created and passed to the handler in the concurrent test scenario. This ensures consistency with the updated handler signature.
Also applies to: 146-146
164-165: Storage connection correctly integrated in TestGetLeaseHandlerMemoryUsageThe memory usage test now properly includes the storage connection. This test is particularly relevant to the PR objective as it verifies that memory consumption remains within acceptable limits.
Also applies to: 183-183
238-238: Storage connection correctly integrated in TestKeepaliveHandlerThe KeepaliveHandler test cases now include the mock storage connection, correctly reflecting the updated handler signature.
Also applies to: 243-243
259-259: Storage connection correctly integrated in TestKeepaliveHandlerConcurrentThe concurrent tests for the KeepaliveHandler function now include the mock storage connection, maintaining consistency with the updated architecture.
Also applies to: 271-271
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
internal/application/application.go (1)
31-45: New CreateLease method implementation.The method correctly utilizes the storage connection from the Application struct. However, there seems to be a discrepancy in the metrics label - using
LeaseOperationGetfor what is actually a lease creation operation.Consider updating the metrics label to something more specific to the creation operation:
- metrics.LeaseOperations.WithLabelValues(metrics.LeaseOperationGet, "error").Inc() + metrics.LeaseOperations.WithLabelValues("create", "error").Inc() - metrics.LeaseOperations.WithLabelValues(metrics.LeaseOperationGet, leaseStatus).Inc() + metrics.LeaseOperations.WithLabelValues("create", leaseStatus).Inc()Or define a new constant
LeaseOperationCreatein the metrics package.internal/delivery/http/server_test.go (1)
165-206: Memory usage test needs validationThe memory usage test is a good way to ensure that the application doesn't consume excessive memory when handling multiple leases. However, the 50MB threshold seems arbitrary.
Consider basing this threshold on benchmarks from previous versions or actual performance requirements. Also, consider using a percentage increase rather than an absolute value, which would make the test more robust across different environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/application/application.go(1 hunks)internal/application/application_test.go(1 hunks)internal/application/command/leasemanagement/leaseManagement.go(2 hunks)internal/application/command/leasemanagement/leasemanagement_test.go(1 hunks)internal/config/config.go(2 hunks)internal/delivery/http/server.go(3 hunks)internal/delivery/http/server_test.go(1 hunks)internal/infrastructure/cache/cache.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
internal/application/command/leasemanagement/leasemanagement_test.go (1)
internal/application/command/leasemanagement/leaseManagement.go (1)
CreateLease(17-47)
internal/application/command/leasemanagement/leaseManagement.go (2)
internal/infrastructure/storage/storage.go (1)
Storage(12-16)internal/application/command/leasemanagement/lease.go (1)
Lease(7-12)
internal/delivery/http/server_test.go (5)
internal/config/config.go (2)
Config(28-33)NewConfig(70-103)internal/infrastructure/cache/cache.go (3)
Cache(11-17)New(34-45)LeaseCacheRecord(29-32)internal/application/application.go (2)
Application(15-20)New(22-29)internal/delivery/http/server.go (1)
New(33-37)internal/infrastructure/storage/storage.go (2)
StatusAccepted(8-8)StatusCreated(9-9)
internal/application/application.go (7)
internal/config/config.go (1)
Config(28-33)internal/infrastructure/storage/mock/mock.go (1)
Storage(9-9)internal/infrastructure/storage/storage.go (1)
Storage(12-16)internal/delivery/http/server.go (1)
New(33-37)internal/application/command/leasemanagement/leaseManagement.go (1)
CreateLease(17-47)internal/application/command/leasemanagement/lease.go (1)
Lease(7-12)internal/infrastructure/metrics/metrics.go (3)
LeaseOperations(13-19)LeaseOperationGet(9-9)LeaseOperationProlong(8-8)
internal/delivery/http/server.go (5)
internal/infrastructure/metrics/metrics.go (3)
LeaseOperationDuration(21-28)LeaseOperationGet(9-9)CacheOperations(30-36)example/client.go (1)
Lease(21-26)internal/infrastructure/cache/cache.go (1)
LeaseCacheRecord(29-32)internal/application/command/leasemanagement/leaseManagement.go (2)
CreateLease(17-47)ReviveLease(49-56)internal/infrastructure/storage/storage.go (2)
StatusAccepted(8-8)StatusCreated(9-9)
internal/application/application_test.go (5)
internal/infrastructure/storage/mock/mock.go (1)
Storage(9-9)internal/infrastructure/storage/storage.go (2)
StatusCreated(9-9)StatusAccepted(8-8)internal/application/application.go (1)
New(22-29)internal/infrastructure/cache/cache.go (2)
New(34-45)LeaseCacheRecord(29-32)internal/application/command/leasemanagement/leaseManagement.go (2)
CreateLease(17-47)ReviveLease(49-56)
🪛 GitHub Actions: Docker Build & Push
internal/delivery/http/server_test.go
[error] Failed to unmarshal request body, invalid character 'i' looking for beginning of value.
[error] Failed to parse lease id from string, leaseIDString: invalid-id, strconv.ParseInt: parsing 'invalid-id': invalid syntax.
[error] Failed to parse lease id from string, leaseIDString: , strconv.ParseInt: parsing '': invalid syntax.
internal/application/application_test.go
[error] 75-80: TestApplication_CreateLease/Lease_already_exists failed: Not equal: expected 'accepted', actual 'created'.
[error] 120-120: TestApplication_ReviveLease/Failed_lease_revival failed: An error is expected but got nil.
[error] 80-80: TestApplication_CreateLease/Successful_lease_creation failed: Should be true.
[error] 75-76: TestApplication_CreateLease/Lease_already_exists failed: Not equal: expected 456, actual 123.
🔇 Additional comments (17)
internal/infrastructure/cache/cache.go (1)
29-32: New struct added for lease caching.This
LeaseCacheRecordstruct looks good. It provides a clean way to cache lease status and ID information, which aligns with the PR objective of optimizing memory consumption by reusing the storage connection.internal/config/config.go (2)
35-38: Added pprof profiling configuration.The addition of the
PPROFEnabledfield in theServerCfgstruct is a good improvement for debugging capabilities.
78-80: Configuration setup for pprof.Good implementation. The configuration correctly reads from environment variables with appropriate defaults.
internal/application/application.go (3)
15-20: Improved Application struct with storage connection.Adding
StorageConnectionas a field in theApplicationstruct is a good architectural improvement. This change aligns with the PR objective of making a common connection to the storage adapter, which should help reduce memory consumption by avoiding multiple connections.
22-29: Updated constructor with storage connection parameter.The constructor now properly accepts and initializes the storage connection. This is a good improvement that makes the dependency explicit and allows for better resource management.
47-57: New ReviveLease method implementation.This method correctly implements the lease revival functionality using the storage connection from the Application struct. The error handling and metrics recording are appropriate.
internal/application/command/leasemanagement/leasemanagement_test.go (1)
148-149: Updated test to match new function signature.The test now correctly calls the
CreateLeasefunction with the updated parameter list, which matches the function signature in the relevant code snippets.internal/application/command/leasemanagement/leaseManagement.go (3)
13-15: Note the removal of DefaultLeaseDurationSeconds constantThe
DefaultLeaseDurationSecondsconstant has been removed from this file, and onlyDefaultPrefixremains. Make sure this change is intentional and that any code that previously relied on this constant has been updated.
17-17: Updated function signature to remove thedataparameterThe
CreateLeasefunction signature has been updated to remove thedata []byteparameter. This is a good simplification since the function now uses the lease value directly.
35-35: Using lease value directly for storage creationThe function now passes
[]byte(lease.Value)to the storage connection instead of using a separate data parameter. This is a cleaner approach that maintains the same functionality.internal/delivery/http/server_test.go (1)
23-31: Good test infrastructure setupThe test configuration and application setup are well-structured, making it easy to create consistent test environments.
internal/delivery/http/server.go (4)
11-11: Added pprof for debuggingGood addition of the pprof package for profiling and debugging. This will be useful for identifying performance bottlenecks and memory leaks.
23-26: Added constants for lease managementDefining constants for the default lease TTL header and duration is a good practice for maintainability.
46-48: Conditional pprof endpoint activationGood implementation of conditionally enabling the pprof endpoint based on configuration. This prevents exposing sensitive debugging information in production environments.
163-165: Simplified health endpointThe health endpoint has been simplified to just return a 200 status code. This is appropriate for a basic health check.
internal/application/application_test.go (2)
128-161: Good concurrent operations testThe concurrent operations test is well designed to verify that the application can handle multiple simultaneous lease operations. This is important for real-world scenarios where the application will receive many requests.
162-181: Testing with cache disabledGood addition of a test for the cache-disabled scenario. This ensures that the application works correctly even when no cache is available.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
29-31: New Environment Variable ConfigurationsThe documentation now introduces three key environment variables:
- SHARED_LOCK_PPROF_ENABLED enables pprof debugging.
- SHARED_LOCK_STORAGE_TYPE lets users choose between
etcdandmockstorage adapters.- SHARED_LOCK_ETCD_ADDR_LIST now uses
http://localhost:2379(changed fromlocalhost:2379), which clarifies the expected URL scheme.Please verify that the application’s configuration parsing correctly handles the HTTP scheme in the etcd address and that these changes align with the storage adapter updates aimed at reducing memory consumption. Also, note that markdownlint MD034 may flag the bare URL—consider wrapping the URL in angle brackets if needed.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
31-31: Bare URL used
null(MD034, no-bare-urls)
36-37: Addition of In-Memory Cache ConfigurationNew variables for cache control:
- SHARED_LOCK_CACHE_ENABLED allows in-memory caching for leases.
- SHARED_LOCK_CACHE_SIZE specifies the maximum number of cache items.
These additions are clear and contribute to the configurability of caching behavior. For enhanced clarity, consider adding a brief note in the README explaining how enabling the cache impacts memory consumption and interacts with the new storage adapter connection introduced in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
31-31: Bare URL used
null
(MD034, no-bare-urls)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/infrastructure/storage/mock/mock.go (1)
43-48: Improve documentation for special lease ID caseThe code returns an error for the specific lease ID
999but doesn't document why this special case exists. This appears to be for testing the failure path inTestApplication_ReviveLease.Consider adding a comment to explain this test-specific behavior:
func (s *Storage) KeepLeaseOnce(ctx context.Context, leaseID int64) error { + // Special case for testing failure scenarios - lease ID 999 is considered not found if leaseID == 999 { return fmt.Errorf("lease not found") } return nil }internal/delivery/http/server_test.go (1)
159-162: Add threshold explanation for memory testThe memory test asserts that allocating 1000 leases should use less than 50MB of memory, but doesn't provide reasoning for this threshold.
Add a comment explaining why 50MB is an appropriate threshold to ensure the test intent is clear and maintainable:
maxMemoryMB := uint64(50 * 1024 * 1024) + // 50MB threshold based on estimated size: 1000 leases × ~1KB per lease structure with overhead assert.LessOrEqual(t, memoryAlloc, maxMemoryMB, "Memory usage should be less than 50MB for 1000 leases")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/application/application_test.go(1 hunks)internal/delivery/http/server_test.go(1 hunks)internal/infrastructure/storage/mock/mock.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal/delivery/http/server_test.go (6)
internal/config/config.go (2)
Config(28-33)NewConfig(70-103)internal/infrastructure/storage/mock/mock.go (2)
Storage(13-16)New(18-22)internal/infrastructure/cache/cache.go (2)
Cache(11-17)New(34-45)internal/application/application.go (2)
Application(15-20)New(22-29)internal/delivery/http/server.go (1)
New(33-37)internal/infrastructure/storage/storage.go (1)
StatusCreated(9-9)
internal/infrastructure/storage/mock/mock.go (2)
internal/application/command/leasemanagement/leaseManagement.go (1)
CreateLease(17-47)internal/infrastructure/storage/storage.go (2)
StatusAccepted(8-8)StatusCreated(9-9)
internal/application/application_test.go (5)
internal/infrastructure/storage/mock/mock.go (3)
Storage(13-16)New(18-22)DefaultPrefix(11-11)internal/infrastructure/storage/storage.go (2)
StatusCreated(9-9)StatusAccepted(8-8)internal/application/application.go (1)
New(22-29)internal/infrastructure/cache/cache.go (1)
New(34-45)internal/application/command/leasemanagement/leaseManagement.go (3)
DefaultPrefix(14-14)CreateLease(17-47)ReviveLease(49-56)
🪛 GitHub Actions: Docker Build & Push
internal/delivery/http/server_test.go
[error] 110-110: Not equal: expected: 201, actual: 202 in TestGetLeaseHandlerConcurrent
[error] 110-110: Failed to parse lease id from string, leaseIDString: invalid-id, strconv.ParseInt: parsing "invalid-id": invalid syntax
[error] 110-110: Failed to parse lease id from string, leaseIDString: , strconv.ParseInt: parsing "": invalid syntax
[warning] 110-110: Can't parse value of x-lease-ttl header. Using defaultLeaseDuration for existing-key
[warning] 110-110: Can't parse value of x-lease-ttl header. Using defaultLeaseDuration for new-key
[warning] 110-110: Can't parse value of x-lease-ttl header. Using defaultLeaseDuration for cache-miss-key
🔇 Additional comments (4)
internal/delivery/http/server_test.go (2)
104-106:⚠️ Potential issueAdd missing header in test requests
The test doesn't set the
x-lease-ttlheader for the request, which results in warnings in the pipeline logs about using default lease durations.req := httptest.NewRequest(http.MethodPost, "/lease", bytes.NewBuffer(body)) + req.Header.Set("x-lease-ttl", "1m")Likely an incorrect or invalid review comment.
43-46:Details
❓ Verification inconclusive
Fix inconsistent test expectations
This test case expects a status code of
http.StatusCreated(201) and a body of "123" for a cache hit with an "accepted" status, but the pipeline failure indicates that the actual status is 202.
🏁 Script executed:
#!/bin/bash # Check if the server actually returns 201 or 202 for accepted leases # First, look for the implementation of the handler to see status code mapping rg -A 10 "handleLease.*func" internal/delivery/httpLength of output: 53
Please verify the correct status code for accepted leases in the tests
It appears that the test in
internal/delivery/http/server_test.go(lines 43–46) currently expects a status ofhttp.StatusCreated(201) with a response body"123", yet the pipeline indicates that the actual response is 202 (i.e.http.StatusAccepted). Since our initial automated search for the handler (using"handleLease") did not return any output, we couldn’t pinpoint the exact implementation details. Additional manual verification is needed to confirm whether the server implementation is intentionally returning 202. Once verified, update the test expectation accordingly.Action items:
- Verify with the implementation whether a cache hit with an “accepted” status should return 202.
- If 202 is correct, update the test diff from:
- expectedStatus: http.StatusCreated, + expectedStatus: http.StatusAccepted,- Confirm that this update is consistent with your overall API behavior for accepted leases.
internal/application/application_test.go (2)
98-101: LGTM! Proper error test case for lease revival.The test correctly checks the error case for lease ID 999, which corresponds to the mock implementation in
KeepLeaseOnce.
123-155: LGTM! Great concurrent testing approach.The concurrent test case properly validates both lease creation and revival operations running simultaneously, which is important for verifying thread safety.
internal/delivery/http/server.go
Outdated
| return | ||
| } | ||
|
|
||
| if s.app.LeaseCache != nil { |
There was a problem hiding this comment.
необхідно перенести це в середину application (це не на рівні контролера логіка), контролер не має знати про LeaseCache
There was a problem hiding this comment.
Заніс в application
internal/delivery/http/server.go
Outdated
| return | ||
| } | ||
|
|
||
| if s.app.LeaseCache != nil { |
There was a problem hiding this comment.
необхідно перенести це в середину application (це не на рівні контролера логіка), контролер не має знати про LeaseCache
There was a problem hiding this comment.
Заніс в application
internal/application/application.go
Outdated
| LeaseCache *cache.Cache | ||
| Ctx context.Context | ||
| Config *config.Config | ||
| LeaseCache *cache.Cache |
There was a problem hiding this comment.
є сенс робити все private щоб ззовні від application не було операцій з атрибутами та деталями реалізації application
There was a problem hiding this comment.
Зробив усе приватним
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/delivery/http/server.go (2)
60-106: 🛠️ Refactor suggestionImprove error handling in lease creation.
The error handling for the lease creation is incomplete. When an error occurs during lease creation, you set the status code to 500 but don't provide an error message, making it harder to debug issues.
Apply this diff to improve error handling:
leaseStatus, leaseID, err = s.app.CreateLease(leaseTTL, lease) if err != nil { - w.WriteHeader(http.StatusInternalServerError) + log.Errorf("Failed to create lease: %v", err) + http.Error(w, fmt.Sprintf("Failed to create lease: %v", err), http.StatusInternalServerError) return }
148-152:⚠️ Potential issueFix status code for lease revival failure.
When reviving a lease fails, you're returning HTTP 204 (No Content) which indicates success with no content. This is misleading for clients as they'll think the operation succeeded.
Apply this diff to fix the issue:
err = s.app.ReviveLease(leaseID) if err != nil { log.Warnf("Failed to prolong lease: %v", err) - http.Error(w, "Failed to prolong lease", http.StatusNoContent) + http.Error(w, "Failed to prolong lease", http.StatusInternalServerError) return }
🧹 Nitpick comments (5)
README.md (3)
30-30: New Environment Variable for pprof Debugging
The addition ofSHARED_LOCK_PPROF_ENABLEDwith a default value offalseis clear and beneficial for toggling pprof-based debugging. Make sure that if enabled in production, proper security precautions are in place.
32-32: Updated etcd Address Format
Changing the default forSHARED_LOCK_ETCD_ADDR_LISTfromlocalhost:2379tohttp://localhost:2379clarifies the required protocol. Note that markdownlint may flag this as a bare URL (MD034); you might consider formatting it as inline code (e.g.,`http://localhost:2379`) for consistency.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
32-32: Bare URL used
null(MD034, no-bare-urls)
38-38: New Environment Variable for Cache Size Limit
AddingSHARED_LOCK_CACHE_SIZEwith a default value of1000provides configurability for cache capacity. This is a useful feature to help manage memory usage. Verify that the default value offers a good balance between performance and resource utilization.internal/delivery/http/server.go (2)
11-11: Consider explicitly using pprof handlers.The blank import for pprof registers handlers with the default ServeMux, but it's generally better to explicitly register the handlers you need for better control and clarity.
Instead of the blank import, consider explicitly importing and registering specific pprof handlers:
-import ( - _ "net/http/pprof" -) +import ( + "net/http/pprof" +) // Then in your Start method: + if cfg.PPROFEnabled { + mux.HandleFunc("/debug/pprof/", pprof.Index) + mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) + mux.HandleFunc("/debug/pprof/profile", pprof.Profile) + mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) + mux.HandleFunc("/debug/pprof/trace", pprof.Trace) + }
111-124: Consider validating leaseID before writing response.For non-error cases, the code writes the leaseID to the response without checking if it's valid (could be 0 for accepted leases). You might want to include additional context or validation.
Consider adding more context or a check:
switch leaseStatus { case storage.StatusAccepted: w.WriteHeader(http.StatusAccepted) case storage.StatusCreated: w.WriteHeader(http.StatusCreated) default: w.WriteHeader(http.StatusInternalServerError) } + // For accepted leases, leaseID will be 0 + responseBody := fmt.Sprintf("%v", leaseID) + if leaseStatus == storage.StatusAccepted { + responseBody = "Lease already exists" + } - _, err = w.Write([]byte(fmt.Sprintf("%v", leaseID))) + _, err = w.Write([]byte(responseBody)) if err != nil { log.Errorf("Failed to write response for /lease endpoint, %v", err) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)internal/application/application.go(1 hunks)internal/config/config.go(3 hunks)internal/delivery/http/server.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/config.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/delivery/http/server.go (4)
internal/infrastructure/metrics/metrics.go (2)
LeaseOperationDuration(21-28)LeaseOperationGet(9-9)example/client.go (1)
Lease(21-26)internal/application/command/leasemanagement/leaseManagement.go (2)
CreateLease(17-47)ReviveLease(49-56)internal/infrastructure/storage/storage.go (2)
StatusAccepted(8-8)StatusCreated(9-9)
internal/application/application.go (4)
internal/infrastructure/cache/cache.go (3)
Cache(11-17)New(34-45)LeaseCacheRecord(29-32)internal/infrastructure/storage/storage.go (1)
Storage(12-16)internal/application/command/leasemanagement/leaseManagement.go (2)
CreateLease(17-47)ReviveLease(49-56)internal/infrastructure/metrics/metrics.go (2)
LeaseOperations(13-19)CacheOperations(30-36)
🪛 markdownlint-cli2 (0.17.2)
README.md
32-32: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (11)
README.md (2)
31-31: New Environment Variable for Storage Type Selection
SHARED_LOCK_STORAGE_TYPEhas been added to allow users to specify the storage mechanism (eitheretcdormock). The description is concise and informative. Consider providing an example in the documentation if users might be unfamiliar with the options.
37-37: New Environment Variable to Enable Cache
The introduction ofSHARED_LOCK_CACHE_ENABLEDclearly indicates the option to use an in-memory cache for leases. This aligns with the objective of optimizing memory consumption. The documentation is straightforward.internal/application/application.go (6)
16-19: Field naming follows encapsulation principles.The change from exported (capitalized) field names to unexported (lowercase) field names is a good practice. This aligns with the principle of encapsulation, keeping the internal details of the
Applicationstruct private and preventing external manipulation.
22-28: Storage connection integration looks good.Adding the storage connection as a parameter to the constructor and storing it in the struct is a good approach. This allows the application to manage a single storage connection instance, which should help reduce memory consumption as mentioned in the PR objectives.
31-45: New CreateLease method handles delegation well.The implementation correctly delegates to the
leasemanagement.CreateLeasefunction, properly handles errors, and updates metrics appropriately. This encapsulation of lease creation logic within the application layer is a good design choice.
47-57: ReviveLease method implementation is correct.The method properly delegates to the appropriate leasemanagement function and handles error cases correctly with appropriate metrics tracking.
59-69: Cache presence check handles edge cases.The method correctly handles the case where the cache is nil, which is a good defensive programming practice.
84-93: AddLeaseToCache method implementation is correct.The method properly handles the nil cache case and correctly sets the cache entry with the appropriate TTL.
internal/delivery/http/server.go (3)
22-25: Good use of constants for configuration values.Defining constants for headers and default durations improves code maintainability and readability.
45-47: PPROF handler registration looks good.The conditional registration of the pprof handler based on the configuration flag is a good practice.
155-157: Simplified health endpoint looks good.The simplified health endpoint is cleaner and more straightforward.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/application/application.go (1)
59-69: Cache presence check handles nil cache properly.The method correctly checks for lease presence in the cache and handles the case where the cache might be nil. However, the error return value seems unnecessary since no error conditions are actually being returned.
Consider simplifying the method signature to just return a boolean since no error conditions are being returned:
-func (a *Application) CheckLeasePresenceInCache(key string) (bool, error) { +func (a *Application) CheckLeasePresenceInCache(key string) bool { if a.leaseCache == nil { - return false, nil + return false } if _, exists := a.leaseCache.Get(key); exists { - return true, nil + return true } - return false, nil + return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/application/application.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/application/application.go (3)
internal/infrastructure/cache/cache.go (2)
Cache(11-17)LeaseCacheRecord(29-32)internal/infrastructure/storage/storage.go (1)
Storage(12-16)internal/application/command/leasemanagement/leaseManagement.go (2)
CreateLease(17-47)ReviveLease(49-56)
🔇 Additional comments (6)
internal/application/application.go (6)
16-19: Appropriate naming convention change to private fields.The fields have been renamed from PascalCase to camelCase, making them private as suggested in the previous review. This change properly encapsulates the internal implementation details of the Application struct.
22-28: Good integration of storage connection into the Application struct.The constructor now accepts a storage connection parameter, which aligns with the PR objective to use a common connection to reduce memory consumption. The initialization of all fields follows a consistent pattern.
31-45: Well-implemented lease creation with proper error handling.The CreateLease method correctly delegates to leasemanagement.CreateLease using the application's storage connection. The error handling, metrics collection, and return values are appropriately managed.
47-57: Proper implementation of lease revival functionality.The ReviveLease method correctly delegates to leasemanagement.ReviveLease and includes appropriate error handling and metrics collection.
71-82: Fixed logging placement in GetLeaseFromCache.The debug log and metrics increment are now correctly placed before the return statement, fixing the issue mentioned in the previous review where they were unreachable when a cache hit occurred.
84-93: Cache addition handles nil cache properly.The method correctly adds a lease to the cache with the specified parameters and handles the case where the cache might be nil. The implementation is clean and straightforward.
Summary by CodeRabbit
New Features
Refactor
Chores
These improvements deliver a more robust and easier-to-monitor application experience.