sdk-generator icon indicating copy to clipboard operation
sdk-generator copied to clipboard

replace errgroup with conc pool in WriteExecute method

Open AltuisticIsopod opened this issue 2 months ago • 2 comments

Replace errgroup with conc pool in WriteExecute method

Description

  • Replace errgroup with conc pool for parallel write/delete operations
  • Use errors.As() instead of type assertion for authentication error detection
  • Handle wrapped errors from conc pool using errors.As() in both client and test
  • Add nil check before dereferencing singleResponse to prevent panics
  • Update test to use errors.As() for proper error unwrapping

This PR only moved "Write" to use Conc

References

https://github.com/openfga/go-sdk/issues/193

Review Checklist

  • [x] I have clicked on "allow edits by maintainers".
  • [x] I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • [x] The correct base branch is being used, if not main
  • [x] I have added tests to validate that the change in functionality is working as expected

Summary by CodeRabbit

  • Refactor
    • Enhanced concurrent request processing with improved error handling and resource management for better reliability and performance.

AltuisticIsopod avatar Oct 26 '25 07:10 AltuisticIsopod

Walkthrough

Replaces per-chunk parallel execution from an errgroup-based approach to a pool-based concurrency model in the Go client template. Introduces controlled worker pools with context propagation, refactors authentication error handling via errors.As, and updates WriteExecute and DeleteExecute methods to iterate over chunks through pool tasks.

Changes

Cohort / File(s) Summary
Go Client Concurrency Refactoring
config/clients/go/template/client/client.mustache
Replaces errgroup-based per-chunk parallelism with worker pools (writePool/deletePool); adds errors package import for error inspection; converts authentication error handling from type assertion to errors.As; refactors WriteExecute and DeleteExecute to collect per-chunk responses via pool tasks instead of explicit index-based errgroup capturing; shifts result aggregation from errgroup.Wait() to pool.Wait()

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Client
    participant OldErrGroup as Old: ErrGroup
    participant NewPool as New: Worker Pool
    participant Chunk

    Client->>OldErrGroup: WriteExecute(chunks)
    loop Per Chunk (indexed)
        OldErrGroup->>Chunk: Go(i) → execute chunk[i]
        Chunk-->>OldErrGroup: response
    end
    OldErrGroup->>OldErrGroup: Wait()
    OldErrGroup-->>Client: combined result + error

    Client->>NewPool: WriteExecute(chunks)
    rect rgb(200, 220, 250)
        Note over NewPool: Initialize pool with<br/>max goroutines
    end
    loop Per Chunk (no index capture)
        NewPool->>Chunk: Go(task) → execute chunk
        Chunk-->>NewPool: response (collected)
    end
    NewPool->>NewPool: Wait()
    NewPool-->>Client: combined result + error
    
    rect rgb(240, 200, 200)
        Note over NewPool: Auth errors via errors.As<br/>instead of type assertion
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Concurrency pattern shift: Review the pool initialization, goroutine limit configuration, and correctness of chunk iteration and response collection logic
  • Error handling refactoring: Verify errors.As correctly replaces the previous type assertion for authentication errors and maintains existing error semantics
  • Response aggregation: Ensure per-chunk responses are properly accumulated and the final combined result construction remains functionally equivalent to the errgroup approach
  • Context propagation: Confirm context is correctly threaded through pool tasks and cleanup behavior is preserved

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "replace errgroup with conc pool in WriteExecute method" accurately describes the primary refactoring in the changeset. The title is specific and clear, correctly identifying the main change as a replacement of the concurrency mechanism from errgroup to a pool-based approach in the WriteExecute method. While the changes also include error handling improvements (switching to errors.As) and nil checks, the core objective and most significant change is the concurrency model replacement, which is properly captured in the title. The title is neither vague nor misleading, and it provides sufficient clarity for a developer scanning the repository history to understand the nature of this change.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 26 '25 07:10 coderabbitai[bot]

Hi @AltuisticIsopod, we'll assign someone to review your PR. Thanks.

dyeam0 avatar Nov 03 '25 04:11 dyeam0