Fixed cache implementation #9
Conversation
|
""" WalkthroughThis set of changes refactors the lease presence checking mechanism throughout the application. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Application
participant Storage
Client->>Application: CreateLease(key, ttl)
Application->>Storage: CheckLeasePresence(key)
Storage-->>Application: leaseID (0 if not present)
alt Lease exists (leaseID != 0)
Application-->>Client: Return "accepted", leaseID
else Lease does not exist
Application->>Storage: CreateLease(key, ttl)
Storage-->>Application: leaseID, status
Application->>Application: Add to lease cache if status is StatusCreated
Application-->>Client: Return status, leaseID
end
Possibly related PRs
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)internal/application/application.go(2 hunks)internal/application/application_test.go(2 hunks)internal/application/command/leasemanagement/leaseManagement.go(1 hunks)internal/application/command/leasemanagement/leasemanagement_test.go(8 hunks)internal/infrastructure/storage/etcd/etcd.go(1 hunks)internal/infrastructure/storage/mock/mock.go(1 hunks)internal/infrastructure/storage/storage.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal/application/application.go (2)
internal/infrastructure/metrics/metrics.go (2)
LeaseOperations(13-19)LeaseOperationGet(9-9)internal/infrastructure/storage/storage.go (1)
StatusCreated(9-9)
internal/application/application_test.go (2)
internal/infrastructure/cache/cache.go (1)
LeaseCacheRecord(29-32)internal/infrastructure/storage/storage.go (1)
StatusCreated(9-9)
internal/infrastructure/storage/mock/mock.go (2)
internal/infrastructure/storage/storage.go (3)
Storage(12-16)StatusAccepted(8-8)StatusCreated(9-9)internal/application/command/leasemanagement/leaseManagement.go (2)
DefaultPrefix(14-14)CreateLease(17-46)
🔇 Additional comments (15)
README.md (1)
78-78: Correct minor markdown formatting for the "Keep Alive Lease" section.The trailing backslash has been removed so that the
- **Responses**:heading renders correctly in Markdown.internal/application/application.go (3)
35-37: Improved metric collection with deferred anonymous functionThe change to use a deferred anonymous function ensures metrics correctly capture the final
leaseStatusvalue, even in error paths. This provides more accurate operation metrics compared to directly callingInc().
46-46: Return actual leaseID instead of zero on errorGood improvement to return the actual
leaseIDrather than hardcoded zero when an error occurs. This provides more accurate information to callers about which lease encountered the error.
49-52: Cache optimization with conditional additionThe optimization to only cache leases with
StatusCreatedstatus is a good improvement. This prevents unnecessary cache entries for leases that didn't complete creation successfully. The added debug log also improves observability.internal/application/application_test.go (2)
55-55: Standardized expectedID valueThe change to use
123as the expected ID in this test case makes it consistent with the ID used in the cache setup below.
68-71: Improved test setup using lease cacheThis change properly simulates a cache hit for an existing lease, which better reflects the actual application behavior. The test now directly populates the cache with a lease record containing the same ID expected in the test assertion.
internal/application/command/leasemanagement/leaseManagement.go (2)
25-25: Improved lease presence check with direct ID returnGood refactoring to get the lease ID directly rather than a boolean flag. This provides more information and avoids additional lookups when the ID is needed.
29-30: Better conditional logic using lease IDThe change from a boolean check to
leaseID != 0is more idiomatic and directly returns the found lease ID instead of zero. This makes the code more consistent and simplifies lease tracking.internal/infrastructure/storage/storage.go (1)
13-13: Improved Storage interface with direct lease ID returnThis is an excellent architectural improvement to the
CheckLeasePresencemethod. Returning the actual lease ID instead of a boolean flag:
- Provides more information to callers
- Eliminates the need for separate lookups when the ID is needed
- Makes the API more consistent with the rest of the storage operations
This change drives all the other improvements throughout the codebase.
internal/infrastructure/storage/etcd/etcd.go (1)
68-85: Return type change for CheckLeasePresence appears correctThe method has been refactored to return the actual
leaseID(int64) instead of a boolean presence flag. This change provides more meaningful information to callers while maintaining the same error handling pattern. The implementation correctly initializesleaseIDto 0 (Go's zero value) and extracts the lease ID from the etcd response when the key exists.internal/application/command/leasemanagement/leasemanagement_test.go (3)
13-24: Mock interface correctly updated for new return typeThe
MockStoragestruct and itsCheckLeasePresencemethod have been properly updated to return(int64, error)instead of(bool, error). The default return value of 0 (instead of false) is appropriate as the zero value for an int64 type.
40-68: Test cases correctly adapted for lease ID return valueThe test cases have been appropriately updated to use
checkLeaseIDinstead of a boolean presence flag. The test for "Lease already exists" now checks for a specific lease ID (456) rather than a boolean, which provides more precise test coverage.
134-146: Mock function implementation correctly updatedThe mock function implementation has been properly updated to return the lease ID value, maintaining the test's behavioral expectations.
internal/infrastructure/storage/mock/mock.go (2)
13-22: Mock storage data structure properly updatedThe
ExistingLeasesmap type has been correctly changed frommap[string]booltomap[string]int64to store lease IDs instead of boolean presence flags, which aligns with the interface changes.
24-32: CheckLeasePresence method correctly returns lease IDThe implementation now returns the actual lease ID if it exists in the map, otherwise it returns 0. This aligns with the updated interface and the etcd implementation.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests