Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds optional post-put version mutation: new Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/storage/interface.go`:
- Around line 137-139: Fix the minor typo in the UpdateVersion method comment:
replace "upate" with "update" in the comment that currently reads "and upate its
item's versions" so the sentence correctly reads "and update its item's
versions"; the comment is adjacent to the UpdateVersion(prevKVs
map[string]*mvccpb.KeyValue) declaration referencing prevKVs and
mvccpb.KeyValue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 266eee22-9a52-4e02-abf1-30b153218a23
📒 Files selected for processing (6)
server/internal/storage/interface.goserver/internal/storage/put.goserver/internal/storage/put_test.goserver/internal/storage/storagetest/ops.goserver/internal/storage/txn.goserver/internal/storage/txn_test.go
💤 Files with no reviewable changes (1)
- server/internal/storage/storagetest/ops.go
jason-lynch
force-pushed
the
refactor/component-logger-constants
branch
from
0eba7c4 to
08cb7d1
Compare
jason-lynch
force-pushed
the
feat/put-with-updated-version
branch
from
fa785be to
64a9954
Compare
| if err != nil { | ||
| return fmt.Errorf("failed to put %q: %w", o.key, err) | ||
| } | ||
| if o.shouldUpdateVersion { |
There was a problem hiding this comment.
Why not call updateVersion() here like createOp.Exec() and updateOp.Exec() do?
There was a problem hiding this comment.
That's a good question. It's just a slightly different output shape. createOp and updateOp use a transaction internally since we need to apply a condition to the operation. That means both of them produce a slice of responses, even though they're only performing one operation. So we're reusing the same code for those as we do with transactions: we iterate over the responses, extract any Put responses, and produce a map[string]*mvccpb.KeyValue of the previous KVs. I considered reusing those functions here, but since they involve a few transformations, it felt cleaner to just duplicate this if/else.
Adds a new feature to `storage.Put` operations to optionally update the value version after the put succeeds. This mechanism works whether the operation is executed by itself or within a transaction. The version is used as a constraint in update operations, so this feature enables us to hold a reference to a value and perform multiple update operations on it without needing to get the value from Etcd in between. We had some unused mock operations in the `storagetest` package, and I opted to remove them in this commit rather than update them for this change.
jason-lynch
force-pushed
the
feat/put-with-updated-version
branch
from
64a9954 to
2db5030
Compare
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 3 |
TIP This summary will be updated as you push new changes. Give us feedback
2 participants
Summary
Adds a new feature to
storage.Putoperations to optionally update the value version after the put succeeds. This mechanism works whether the operation is executed by itself or within a transaction. The version is used as a constraint in update operations, so this feature enables us to hold a reference to a value and perform multiple update operations on it without needing to get the value from Etcd in between.We had some unused mock operations in the
storagetestpackage, and I opted to remove them in this commit rather than update them for this change.Testing
This feature is used by the next PR in this sequence. In this PR, it's only used in the unit/narrow integration tests, so there are no functional changes.