Dual Storage Architecture #5
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Dual writer: mode 3 * Add integration tests for playlits in mode 3 * Remove todo * Update pkg/apiserver/rest/dualwriter_mode3.go Co-authored-by: Arati R. <[email protected]> * Admin: Fixes an issue where user accounts could not be enabled (#88117) Fix: unable to enable user * [REVIEW] FInish mode 3 and add tests * Improve logging * Update dependencies * Update pkg/apiserver/rest/dualwriter_mode3_test.go Co-authored-by: maicon <[email protected]> * remove test assertion * Use mode log when dual writer is initiated --------- Co-authored-by: Arati R. <[email protected]> Co-authored-by: gonvee <[email protected]> Co-authored-by: maicon <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Author
Greptile OverviewConfidence Score: 0/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client
participant DualWriterMode3
participant Storage as Unified Storage
participant Legacy as Legacy Storage
participant Metrics
Note over DualWriterMode3: Mode 3: Write to both, Read from Storage
rect rgb(200, 220, 255)
Note over Client,Metrics: Create/Update/Delete Operations
Client->>DualWriterMode3: Create/Update/Delete(obj)
DualWriterMode3->>Storage: Create/Update/Delete(obj)
Storage-->>DualWriterMode3: result, err
alt Storage Success
DualWriterMode3->>Metrics: recordStorageDuration(success)
DualWriterMode3-->>Client: return result
par Async Legacy Write
DualWriterMode3->>Legacy: Create/Update/Delete(obj)
Legacy-->>DualWriterMode3: result, err
DualWriterMode3->>Metrics: recordLegacyDuration(success/fail)
end
else Storage Error
DualWriterMode3->>Metrics: recordStorageDuration(error)
DualWriterMode3-->>Client: return error
end
end
rect rgb(200, 255, 220)
Note over Client,Metrics: Read Operations (Get/List)
Client->>DualWriterMode3: Get/List()
DualWriterMode3->>Storage: Get/List()
Storage-->>DualWriterMode3: result, err
DualWriterMode3->>Metrics: recordStorageDuration()
DualWriterMode3-->>Client: return result
Note over Legacy: Legacy storage not used for reads
end
|
| created, err := d.Storage.Create(ctx, obj, createValidation, options) | ||
| if err != nil { | ||
| log.Error(err, "unable to create object in storage") | ||
| d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) |
There was a problem hiding this comment.
logic: calling recordLegacyDuration when Storage.Create fails, should be recordStorageDuration
Suggested change
| d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) | |
| d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/apiserver/rest/dualwriter_mode3.go
Line: 45:45
Comment:
**logic:** calling `recordLegacyDuration` when Storage.Create fails, should be `recordStorageDuration`
```suggestion
d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage)
```
How can I resolve this? If you propose a fix, please make it concise.| upstream: objInfo, | ||
| updated: updated, | ||
| log.Error(err, "unable to update in storage") | ||
| d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) |
There was a problem hiding this comment.
logic: calling recordLegacyDuration when Storage.Update fails, should be recordStorageDuration
Suggested change
| d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) | |
| d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/apiserver/rest/dualwriter_mode3.go
Line: 129:129
Comment:
**logic:** calling `recordLegacyDuration` when Storage.Update fails, should be `recordStorageDuration`
```suggestion
d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage)
```
How can I resolve this? If you propose a fix, please make it concise.| d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage) | ||
| return res, async, err | ||
| } | ||
| d.recordStorageDuration(false, mode3Str, name, method, startStorage) |
There was a problem hiding this comment.
logic: recording metric with name parameter instead of options.Kind - inconsistent with other methods
Suggested change
| d.recordStorageDuration(false, mode3Str, name, method, startStorage) | |
| d.recordStorageDuration(false, mode3Str, options.Kind, method, startStorage) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/apiserver/rest/dualwriter_mode3.go
Line: 106:106
Comment:
**logic:** recording metric with `name` parameter instead of `options.Kind` - inconsistent with other methods
```suggestion
d.recordStorageDuration(false, mode3Str, options.Kind, method, startStorage)
```
How can I resolve this? If you propose a fix, please make it concise.| ctx, cancel := context.WithTimeoutCause(ctx, time.Second*10, errors.New("legacy deletecollection timeout")) | ||
| defer cancel() | ||
| _, err := d.Legacy.DeleteCollection(ctx, deleteValidation, options, listOptions) | ||
| d.recordStorageDuration(err != nil, mode3Str, options.Kind, method, startLegacy) |
There was a problem hiding this comment.
logic: calling recordStorageDuration in legacy goroutine, should be recordLegacyDuration
Suggested change
| d.recordStorageDuration(err != nil, mode3Str, options.Kind, method, startLegacy) | |
| d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/apiserver/rest/dualwriter_mode3.go
Line: 166:166
Comment:
**logic:** calling `recordStorageDuration` in legacy goroutine, should be `recordLegacyDuration`
```suggestion
d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy)
```
How can I resolve this? If you propose a fix, please make it concise.| func (d *DualWriterMode3) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { | ||
| var method = "delete" | ||
| log := d.Log.WithValues("name", name, "kind", options.Kind, "method", method) | ||
| ctx = klog.NewContext(ctx, d.Log) |
There was a problem hiding this comment.
logic: using d.Log instead of local log variable that was just created with enriched context
Suggested change
| ctx = klog.NewContext(ctx, d.Log) | |
| ctx = klog.NewContext(ctx, log) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/apiserver/rest/dualwriter_mode3.go
Line: 97:97
Comment:
**logic:** using `d.Log` instead of local `log` variable that was just created with enriched context
```suggestion
ctx = klog.NewContext(ctx, log)
```
How can I resolve this? If you propose a fix, please make it concise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test 6
Replicated from ai-code-review-evaluation/grafana-greptile#6