[orion-server] feat: add buck2 log segment function#1332
Conversation
Signed-off-by: Xiaoyang Han <lux1an@qq.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: Xiaoyang Han <lux1an@qq.com>
Signed-off-by: Xiaoyang Han <lux1an@qq.com>
Signed-off-by: Xiaoyang Han <lux1an@qq.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new log segment reading feature to the orion-server, enabling clients to request specific byte ranges from task log files. The implementation includes both the core functionality and comprehensive test coverage.
- Introduces
read_log_segment_rawfunction to read log file segments by offset and length - Adds new API endpoint
/task-output-segment/{id}for HTTP access to log segments - Enhances task status handling with a new "Pending" state and
/tasksendpoint
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| orion-server/src/scheduler.rs | Core log segment reading logic, error types, and enhanced build log directory handling |
| orion-server/src/api.rs | New API endpoint for log segments, task status improvements, and tasks listing endpoint |
| orion-server/src/server.rs | Updated OpenAPI schema registration for new types |
| orion-server/tests/log_segment_api.rs | Comprehensive integration tests for log segment functionality |
| orion-server/src/lib.rs | Module exports for library structure |
| orion-server/Cargo.toml | Added tempfile dev dependency for testing |
| ceres/src/api_service/mono_api_service.rs | Code formatting improvements (unrelated to main feature) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let tmp = tempfile::tempdir().expect("temp dir"); | ||
| unsafe { | ||
| std::env::set_var("BUILD_LOG_DIR", tmp.path().to_str().unwrap()); | ||
| } |
There was a problem hiding this comment.
Using unsafe to set environment variables is unnecessary and potentially problematic. Consider using std::env::set_var directly without the unsafe block, as set_var is safe to call.
| } | |
| std::env::set_var("BUILD_LOG_DIR", tmp.path().to_str().unwrap()); |
| let tmp = tempfile::tempdir().unwrap(); | ||
| unsafe { | ||
| std::env::set_var("BUILD_LOG_DIR", tmp.path().to_str().unwrap()); | ||
| } |
There was a problem hiding this comment.
Using unsafe to set environment variables is unnecessary. std::env::set_var is a safe function and doesn't require an unsafe block.
| } | |
| std::env::set_var("BUILD_LOG_DIR", tmp.path().to_str().unwrap()); |
| unsafe { | ||
| std::env::set_var("BUILD_LOG_DIR", tmp.path().to_str().unwrap()); | ||
| } | ||
| let task_id = "segment-oob"; |
There was a problem hiding this comment.
Using unsafe to set environment variables is unnecessary. std::env::set_var is a safe function and doesn't require an unsafe block.
| let task_id = "segment-oob"; | |
| std::env::set_var("BUILD_LOG_DIR", tmp.path().to_str().unwrap()); | |
| let task_id = "segment-oob"; |
| BUILD_LOG_DIR_TLS.with(|cell| { | ||
| if cell.get().is_none() { | ||
| let val = std::env::var("BUILD_LOG_DIR").expect("BUILD_LOG_DIR must be set"); | ||
| let leaked: &'static str = Box::leak(val.into_boxed_str()); |
There was a problem hiding this comment.
[nitpick] The comment mentions 'bounded leak acceptable in tests' but doesn't explain why this approach was chosen over alternatives like Arc<String> or Rc<String>. Consider adding more detail about the trade-offs that led to this design decision.
No description provided.