Expedite spec recommendation by fully using db query#2116
Expedite spec recommendation by fully using db query#2116cb-github-robot merged 3 commits intocloud-barista:mainfrom
Conversation
Signed-off-by: Seokho Son <shsongist@gmail.com>
Signed-off-by: Seokho Son <shsongist@gmail.com>
Signed-off-by: Seokho Son <shsongist@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the spec recommendation system by moving from CSV-based to database-driven latency calculations. The changes include migrating runtime latency data to a database table, implementing database query optimization with indexes, and enhancing the filtering API with limit functionality for better performance.
- Replaces CSV file-based latency lookups with database table queries for improved performance
- Adds comprehensive database indexes for spec filtering and latency queries
- Implements limit parameter in filtering API to control result set size
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.go | Migrates latency data from CSV to database and adds optimized indexes |
| src/interface/rest/server/resource/spec.go | Updates API to support limit parameter for result pagination |
| src/interface/rest/docs/ | Updates API documentation to reflect limit parameter changes |
| src/core/resource/spec.go | Enhances filtering with database query optimization and limit support |
| src/core/model/spec.go | Adds TbLatencyInfo model and database operations for latency data |
| src/core/infra/recommendation.go | Refactors to use database-based sorting and latency lookups |
| src/core/infra/provisioning.go | Minor variable naming improvements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| defer func() { | ||
| if closeErr := file.Close(); closeErr != nil { | ||
| log.Warn().Err(closeErr).Msg("Failed to close CSV file") | ||
| } | ||
| if v[0] == "" { | ||
| }() |
There was a problem hiding this comment.
The deferred function captures closeErr in a closure but doesn't handle the original error from file.Close(). Consider using defer file.Close() directly or handle both errors appropriately.
| likeSearchFields := []string{ | ||
| "AcceleratorModel", // e.g., "NVIDIA H100" -> search with "NVIDIA" | ||
| "Description", // Description text partial search | ||
| // Note: AcceleratorType removed - uses exact matching for better performance | ||
| // since values are typically standardized (GPU, TPU, etc.) | ||
| } | ||
|
|
There was a problem hiding this comment.
The likeSearchFields slice is created on every function call. Consider moving this to a package-level variable or constant to avoid repeated allocations.
| likeSearchFields := []string{ | |
| "AcceleratorModel", // e.g., "NVIDIA H100" -> search with "NVIDIA" | |
| "Description", // Description text partial search | |
| // Note: AcceleratorType removed - uses exact matching for better performance | |
| // since values are typically standardized (GPU, TPU, etc.) | |
| } |
| latencySubquery := fmt.Sprintf(` | ||
| COALESCE(( | ||
| SELECT latency_ms | ||
| FROM tb_latency_infos | ||
| WHERE source_region = '%s' | ||
| AND target_region = tb_spec_infos.provider_name || '+' || tb_spec_infos.region_name | ||
| LIMIT 1 | ||
| ), 999999)`, targetRegion) |
There was a problem hiding this comment.
Using fmt.Sprintf to build SQL queries with user input can lead to SQL injection vulnerabilities. Consider using parameterized queries or proper SQL escaping for the targetRegion parameter.
| latencySubquery := fmt.Sprintf(` | |
| COALESCE(( | |
| SELECT latency_ms | |
| FROM tb_latency_infos | |
| WHERE source_region = '%s' | |
| AND target_region = tb_spec_infos.provider_name || '+' || tb_spec_infos.region_name | |
| LIMIT 1 | |
| ), 999999)`, targetRegion) | |
| ), 999999)`, safeTargetRegion) |
| vmObj, err := GetVmObject(nsId, mciId, vmIdList[0]) | ||
| if err != nil { | ||
| temp := &model.TbMciInfo{} | ||
| return temp, err | ||
| } |
There was a problem hiding this comment.
There's a potential index out of bounds error if vmIdList is empty. The code should check len(vmIdList) > 0 before accessing vmIdList[0].
|
/approve |
speed up
ex)
from
elapsedTime=2179.311783 ms
to
elapsedTime=117.170231 ms