perf(azure database): batched per-server lookup for AZConfig (closes #149)#812
perf(azure database): batched per-server lookup for AZConfig (closes #149)#812cristim wants to merge 2 commits into
Conversation
cristim
added
triaged
|
Warning Review limit reached
More reviews will be available in 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai review |
Rate Limit Exceeded
|
|
@coderabbitai full review Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and files changed
gh pr view 812 --repo LeanerCloud/CUDly --json title,body,files,additions,deletions,commits
echo "=== PR DIFF ==="
gh pr diff 812 --repo LeanerCloud/CUDlyLength of output: 24586
🔴 Medium — Misleading log message when a page walk partially succeedsFile: When a page fetch fails mid-walk, the warning says "signal unavailable", but the function returns the partial counts it collected up to that point. Example: page 1 returns 3 zone-redundant instances, page 2 errors → Choose one of two consistent paths: Option A — document partial-data use (optimistic): logging.Warnf("azure database: managed instances page fetch failed after %d instances: %v — AZConfig/Deployment derived from partial data", total, err)
return zoneRedundant, nonZoneRedundant, totalOption B — zero out on any error (conservative, matches the "unavailable" wording): logging.Warnf("azure database: managed instances page fetch failed: %v — AZConfig/Deployment signal unavailable", err)
return 0, 0, 0The same choice applies to the equivalent warning in 🟡 Low — Missing test for the mixed-deployment branch
func TestDatabaseClient_ConvertAzureSQLRecommendation_DeploymentAmbiguous(t *testing.T) {
client := NewClient(nil, "test-subscription", "eastus")
client.SetCapabilitiesClient(&MockCapabilitiesClient{})
zr := true
client.SetManagedInstancesPager(&MockSQLManagedInstancesPager{
pages: []armsql.ManagedInstancesClientListResponse{
{ManagedInstanceListResult: armsql.ManagedInstanceListResult{
Value: []*armsql.ManagedInstance{
{Properties: &armsql.ManagedInstanceProperties{ZoneRedundant: &zr}},
},
}},
},
})
// Also has regular servers → mixed → Deployment=""
client.SetServersPager(&MockSQLServersPager{
pages: []armsql.ServersClientListResponse{buildServerPage(1)},
})
rec := mocks.BuildLegacyReservationRecommendation(mocks.WithNormalizedSize("GeneralPurpose_Gen5_2"))
out := client.convertAzureSQLRecommendation(context.Background(), rec)
require.NotNil(t, out)
details, ok := out.Details.(common.DatabaseDetails)
require.True(t, ok)
assert.Empty(t, details.Deployment, "mixed managed+regular must leave Deployment empty")
}ℹ️ Informational —
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…loses #149) Replace deferred per-server AZConfig and Deployment lookups with two subscription-wide batched pager walks (managed instances + servers), gated by a single sync.Once so the pagers are each walked at most once per GetRecommendations call regardless of recommendation count. Before: 0 SDK calls (fields left empty, deferred). After: 2 SDK calls total for any N recommendations -- one ManagedInstancesClient.NewListPager walk and one ServersClient.NewListPager walk, both cached for the client lifetime. Adds 8 unit tests covering: zoneRedundant/none/ambiguous AZConfig, managed/single/error Deployment, pager-error fallback, and the _FetchedOnce invariant pinning the 2-call guarantee.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
GetRecommendationscall viasync.Once.ManagedInstancesClient.NewListPager+ServersClient.NewListPager, both cached for the client lifetime.ManagedInstanceProperties.ZoneRedundant("zoneRedundant" / "none" / "" when ambiguous). Deployment derived from managed-instance vs. regular-server presence ("managed" / "single" / "").Test plan
_PopulatesAZConfig: all zone-redundant managed instances ->AZConfig="zoneRedundant"_AmbiguousAZConfig: mixed zone-redundancy ->AZConfig=""_AZConfigPagerErrorFallsBack: pager error ->AZConfig="", conversion still succeeds_PopulatesDeployment: only managed instances ->Deployment="managed"_DeploymentSingle: only regular servers, no managed instances ->Deployment="single"_DeploymentPagerErrorFallsBack: servers pager error ->Deployment="", conversion still succeeds_ServerInfo_FetchedOnce: 10 converter calls -> managed-instances pager walked exactly oncego build ./...clean