fix: increase channel in LO to 10% of the max results#2735
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 WalkthroughAdjusts list object buffering by computing channel capacity dynamically based on max results, and documents the change in the changelog. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as Server: ListObjectsQuery
participant R as ReverseExpand Worker
C->>S: ListObjects request
Note over S: Compute bufferSize = 10 by default<br/>If maxResults set and <1000:<br/>bufferSize = max(10, maxResults/10)
S->>S: Create reverseExpandResultsChan(bufferSize)
S->>R: Start reverse expand processing
R-->>S: Send ReverseExpandResult (buffered)
S-->>C: Stream results (until done)
Note over S,R: Backpressure governed by dynamic buffer
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2735 +/- ##
==========================================
- Coverage 90.12% 90.08% -0.04%
==========================================
Files 144 144
Lines 19928 19906 -22
==========================================
- Hits 17959 17931 -28
- Misses 1493 1502 +9
+ Partials 476 473 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5ff5bac to
05a076b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
12-12: Clarify the buffer sizing logic in the changelog.The changelog states the buffer is "10% of the max results or 10", but the implementation only applies the 10% sizing when
listObjectsMaxResults < 1000. ForlistObjectsMaxResults >= 1000or unlimited (0), the buffer remains fixed at 10. Consider updating the changelog to reflect this threshold.Apply this diff to provide a more accurate description:
-- Increase the buffer size for LO requests to be 10% of the max results returned or 10. [#2735](https://github.com/openfga/openfga/pull/2735) +- Increase the buffer size for LO requests to 10% of the max results (minimum 10) when max results is below 1000, otherwise 10. [#2735](https://github.com/openfga/openfga/pull/2735)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)pkg/server/commands/list_objects.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/server/commands/list_objects.go (1)
pkg/server/commands/reverseexpand/reverse_expand.go (1)
ReverseExpandResult(219-222)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: go-bench
- GitHub Check: docker-tests
- GitHub Check: tests
- GitHub Check: Dry Run GoReleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
pkg/server/commands/list_objects.go (1)
324-332: Verify the buffer size discontinuity at the 1000 threshold.There's a sharp discontinuity in the buffer sizing logic:
- For
maxResults = 999: buffer = 99- For
maxResults = 1000: buffer = 10This 10x drop might not be intentional. Consider whether the threshold of 1000 is the right approach, or if a capped scaling would be better.
If the discontinuity is intentional (e.g., to prevent excessive memory usage), consider adding a comment explaining the rationale. Otherwise, you might want to smooth the logic with a cap:
bufferSize := 10 -if q.listObjectsMaxResults > 0 && q.listObjectsMaxResults < 1000 { - bufferSize = int(q.listObjectsMaxResults / 10) // 10% of max results - if bufferSize < 10 { - bufferSize = 10 - } +if q.listObjectsMaxResults > 0 { + calculatedSize := int(q.listObjectsMaxResults / 10) + if calculatedSize > 10 { + bufferSize = min(100, calculatedSize) // cap at 100 to prevent excessive memory + } }
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
1 similar comment
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @poovamraj's task —— View job Review Progress
Overall Assessment❌ The PR contains logic that doesn't match its description and CHANGELOG. The implementation calculates |
Description
What problem is being solved?
The buffer size for LO is quite low for the default max results size of 1000. Increased the buffer to be 10% of the max results, capping the max results at 1000, and mining the buffer size to 10.
References
Review Checklist
mainSummary by CodeRabbit
Bug Fixes
Documentation