replace errgroup with conc pool in WriteExecute method #249
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes replace concurrent error-group parallelism with a pool-based approach in batch write and delete operations, add four new public methods to the SdkClient interface for managing authorization model and store IDs, and improve error handling by using the errors.As package for safer type assertions across the client and test code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes The changes introduce multiple distinct patterns: a significant parallelism refactor in critical write/delete paths requiring verification of pool-worker error flow, new public API methods with straightforward validation logic, and consistent but distributed error handling improvements across two files that each require separate reasoning. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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[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 (3)
client/client_test.go (1)
1738-1740: Align this test with errors.As for consistencyUse errors.As here too to avoid panics on wrapped errors.
- if _, ok := err.(openfga.FgaApiAuthenticationError); !ok { - t.Fatalf("Expected an api auth error") - } + var authErr openfga.FgaApiAuthenticationError + if !errors.As(err, &authErr) { + t.Fatalf("Expected an api auth error") + }client/client.go (2)
1792-1823: Cancel peers on first auth errorPools don’t cancel sibling tasks; previous errgroup did via context. Add WithCancelOnError so an auth failure stops in-flight work.
- writePool := pool.NewWithResults[*ClientWriteResponse]().WithContext(request.GetContext()).WithMaxGoroutines(int(maxParallelReqs)) + writePool := pool.NewWithResults[*ClientWriteResponse](). + WithContext(request.GetContext()). + WithMaxGoroutines(int(maxParallelReqs)). + WithCancelOnError() - deletePool := pool.NewWithResults[*ClientWriteResponse]().WithContext(request.GetContext()).WithMaxGoroutines(int(maxParallelReqs)) + deletePool := pool.NewWithResults[*ClientWriteResponse](). + WithContext(request.GetContext()). + WithMaxGoroutines(int(maxParallelReqs)). + WithCancelOnError()Also applies to: 1837-1872
1830-1834: Use deleteChunkSize when chunking deletesCurrently uses writeChunkSize; same value today, but confusing.
- end := int(math.Min(float64(i+writeChunkSize), float64(len(request.GetBody().Deletes)))) + end := int(math.Min(float64(i+deleteChunkSize), float64(len(request.GetBody().Deletes))))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/client.go(5 hunks)client/client_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/client_test.go (1)
errors.go (1)
FgaApiAuthenticationError(47-60)
client/client.go (1)
errors.go (1)
FgaApiAuthenticationError(47-60)
🔇 Additional comments (2)
client/client_test.go (1)
1779-1781: Good switch to errors.As for auth error unwrappingSafer and matches wrapped errors from conc pool. LGTM.
Also applies to: 1798-1800
client/client.go (1)
1811-1819: Nice: errors.As + nil-check avoid panics and classify auth errors correctlyThis correctly bubbles auth errors while still aggregating non-auth failures per item. LGTM.
Also applies to: 1857-1866
|
|
Hey @rhamzeh, I have replaced errgroup with conc for Write. Can you review and let me know if you find any issues. |
|
Hi @AltuisticIsopod, same as with openfga/sdk-generator#653, we'll assign someone to review your PR. Thanks. |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (33.62%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
+ Coverage 33.58% 33.62% +0.04%
==========================================
Files 115 115
Lines 9808 9829 +21
==========================================
+ Hits 3294 3305 +11
- Misses 6252 6260 +8
- Partials 262 264 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…isticIsopod/go-sdk into task-Move_Write_to_use_conc
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the WriteExecute method to use github.com/sourcegraph/conc/pool instead of golang.org/x/sync/errgroup for parallel processing, and updates error handling to use errors.As() instead of type assertions for checking authentication errors.
Key changes:
- Replaced
errgroupwithpool.NewWithResultsfor managing concurrent write and delete operations - Updated authentication error checking from type assertions (
err.(Type)) toerrors.As()for better error unwrapping support - Updated corresponding test cases to use the new error checking pattern
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| client/client.go | Refactored WriteExecute to use conc pool library for concurrent operations and updated error handling to use errors.As() |
| client/client_test.go | Updated test cases to use errors.As() for authentication error checking |
Comments suppressed due to low confidence (1)
client/client.go:1860
- The logic assumes
writeResponsesanddeleteResponsescontain non-nil pointers, but the pool returns[]*ClientWriteResponse. If any goroutine returnsnilfor the response (which happens in lines 1801 and 1842 when authentication errors occur), this will cause a nil pointer dereference when accessing.Writesor.Deletes. Add nil checks before dereferencing:if writeResponse != nil { response.Writes = append(...) }
for _, writeResponse := range writeResponses {
response.Writes = append(response.Writes, writeResponse.Writes...)
}
for _, deleteResponse := range deleteResponses {
response.Deletes = append(response.Deletes, deleteResponse.Deletes...)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SoulPancake
left a comment
SoulPancake
left a comment
There was a problem hiding this comment.
LG for WriteExecute, Can you possibly also do this for ClientBatchCheck? @AltuisticIsopod
|
Sure, I will do that. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
client/client.go
Outdated
| return &batchCheckResult{ | ||
| Index: index, | ||
| Response: ClientBatchCheckClientSingleResponse{ | ||
| Request: checkBody, | ||
| ClientCheckResponse: *singleResponse, |
There was a problem hiding this comment.
Potential nil pointer dereference. If client.CheckExecute returns a non-authentication error with singleResponse=nil, the code will dereference singleResponse at line 2276 causing a panic. The code should check if singleResponse is nil before dereferencing it, especially since the err is being stored in the response for non-authentication errors.
| return &batchCheckResult{ | |
| Index: index, | |
| Response: ClientBatchCheckClientSingleResponse{ | |
| Request: checkBody, | |
| ClientCheckResponse: *singleResponse, | |
| var clientCheckResponse fgaSdk.ClientCheckResponse | |
| if singleResponse != nil { | |
| clientCheckResponse = *singleResponse | |
| } | |
| return &batchCheckResult{ | |
| Index: index, | |
| Response: ClientBatchCheckClientSingleResponse{ | |
| Request: checkBody, | |
| ClientCheckResponse: clientCheckResponse, |
|
Hi @SoulPancake , The earlier codebase was only checking for errors of type fgaSdk.FgaApiAuthenticationError. So, I followed the same and I am only catching those errors. |
|
@AltuisticIsopod |
|
@AltuisticIsopod Any chance you could add the nil checks? |
|
Sure, I will do that. |
…isticIsopod/go-sdk into task-Move_Write_to_use_conc
|
@SoulPancake I have added nil checks along with the authentication error check. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
client/client.go
Outdated
|
|
||
| err = deleteGroup.Wait() | ||
| deleteResponses, err := deletePool.Wait() | ||
| // If an error was returned then it will be an authentication error so we want to return |
There was a problem hiding this comment.
This comment is misleading. The error handling checks specifically for authentication errors, but non-authentication errors can also occur (e.g., validation errors, rate limit errors, internal errors). When a non-authentication error occurs, it will be silently ignored, and a nil singleResponse will be returned. Consider either handling all error types explicitly or updating the comment to reflect the actual behavior.
| // If an error was returned then it will be an authentication error so we want to return | |
| // At this point, any non-nil err will be an authentication error (non-authentication | |
| // errors from chunk operations are ignored in the goroutines), so we return it directly. |
|
Why are we migrating from errgroup to conc? The PR description is not clear about that. |
|
@senojj our reasons for migrating to conc are not major, but we do want consistency - so either conc or errgroup, and have that pattern established for all future requests that do batching. As for why conc over errgroup - it has a better DX and workflow, e.g.
That said, if there's a better argument to be made for errgroup, we can definitely consider sticking with that |
6 participants
Replace errgroup with conc pool in WriteExecute method
Description
This PR only moved "Write" to use Conc
References
#193
Review Checklist
main