getting ready to add TaskPaginatedRequests #27
Conversation
|
Warning Rate limit exceeded@IGN-Styly has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughA new paginated task querying feature was introduced, including a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EngineService
participant Auth
participant TaskStore
Client->>EngineService: GetTasks(TaskPageRequest)
EngineService->>Auth: CheckAuth(request)
alt Auth Success
EngineService->>TaskStore: Retrieve tasks by namespace, task, state
TaskStore-->>EngineService: Task list
EngineService->>EngineService: Paginate and format tasks
EngineService-->>Client: TaskPage (paginated tasks)
else Auth Failure
EngineService-->>Client: PermissionDenied error
end
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
proto/engine.proto (2)
14-14: Remove unused empty message or add fields.The
TaskSelectormessage is defined but empty and not used anywhere in the current implementation. Either remove it if it's not needed, or add the intended fields if this is incomplete work.-message TaskSelector {}
9-9: Consider removing commented code or adding a TODO.The commented-out
DeleteTaskRPC suggests incomplete work. Either implement it, remove it, or add a TODO comment explaining the planned implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
proto/engine.proto(1 hunks)src/bin/server.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/bin/server.rs (1)
src/lib.rs (2)
get_auth(12-19)get_uid(3-10)
🔇 Additional comments (4)
proto/engine.proto (2)
15-19: Good enum design for task states.The
TaskStateenum appropriately represents the task lifecycle with clear, meaningful values.
20-34: Well-structured pagination messages.The
TaskPageRequestandTaskPagemessages are well-designed for pagination with appropriate field types and clear naming conventions.src/bin/server.rs (2)
10-15: Clean import additions.The new imports properly support the added functionality and are well-organized.
35-47: Authentication and error handling look solid.The authentication check using
Events::CheckAuthand proper error responses with logging are well implemented.
| let q: Vec<proto::Task> = match data.clone().state() { | ||
| TaskState::Processing => { | ||
| match api | ||
| .executing_tasks | ||
| .clone() | ||
| .tasks | ||
| .get(&(data.namespace.clone(), data.task.clone())) | ||
| { | ||
| Some(tasks) => { | ||
| let mut d = tasks.clone(); | ||
| d.sort_by_key(|f| f.id.clone()); | ||
| d.iter() | ||
| .map(|f| proto::Task { | ||
| id: f.id.clone(), | ||
| task_id: vec![data.namespace.clone(), data.task.clone()].join(":"), | ||
| task_payload: f.bytes.clone(), | ||
| payload: Vec::new(), | ||
| }) | ||
| .collect() | ||
| } | ||
| None => { | ||
| info!( | ||
| "Namespace {:?} and task {:?} not found in Processing state", | ||
| data.namespace, data.task | ||
| ); | ||
| Vec::new() | ||
| } | ||
| } | ||
| } | ||
| TaskState::Queued => { | ||
| match api | ||
| .task_queue | ||
| .clone() | ||
| .tasks | ||
| .get(&(data.namespace.clone(), data.task.clone())) | ||
| { | ||
| Some(tasks) => { | ||
| let mut d = tasks.clone(); | ||
| d.sort_by_key(|f| f.id.clone()); | ||
| d.iter() | ||
| .map(|f| proto::Task { | ||
| id: f.id.clone(), | ||
| task_id: vec![data.namespace.clone(), data.task.clone()].join(":"), | ||
| task_payload: f.bytes.clone(), | ||
| payload: Vec::new(), | ||
| }) | ||
| .collect() | ||
| } | ||
| None => { | ||
| info!( | ||
| "Namespace {:?} and task {:?} not found in Queued state", | ||
| data.namespace, data.task | ||
| ); | ||
| Vec::new() | ||
| } | ||
| } | ||
| } | ||
| TaskState::Solved => { | ||
| match api | ||
| .solved_tasks | ||
| .clone() | ||
| .tasks | ||
| .get(&(data.namespace.clone(), data.task.clone())) | ||
| { | ||
| Some(tasks) => { | ||
| let mut d = tasks.clone(); | ||
| d.sort_by_key(|f| f.id.clone()); | ||
| d.iter() | ||
| .map(|f| proto::Task { | ||
| id: f.id.clone(), | ||
| task_id: vec![data.namespace.clone(), data.task.clone()].join(":"), | ||
| task_payload: f.bytes.clone(), | ||
| payload: Vec::new(), | ||
| }) | ||
| .collect() | ||
| } | ||
| None => { | ||
| info!( | ||
| "Namespace {:?} and task {:?} not found in Solved state", | ||
| data.namespace, data.task | ||
| ); | ||
| Vec::new() | ||
| } | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication.
The three match arms contain nearly identical logic with only different collection access. This violates DRY principles and makes maintenance difficult.
Consider extracting the common logic:
+fn get_tasks_by_state(
+ tasks: Option<&Vec<impl Clone + TaskLike>>,
+ namespace: &str,
+ task: &str
+) -> Vec<proto::Task> {
+ match tasks {
+ Some(task_list) => {
+ let mut sorted_tasks = task_list.clone();
+ sorted_tasks.sort_by_key(|f| f.id());
+ sorted_tasks.iter()
+ .map(|f| proto::Task {
+ id: f.id(),
+ task_id: format!("{}:{}", namespace, task),
+ task_payload: f.bytes(),
+ payload: Vec::new(),
+ })
+ .collect()
+ }
+ None => {
+ info!("Namespace {:?} and task {:?} not found in requested state", namespace, task);
+ Vec::new()
+ }
+ }
+}
let q: Vec<proto::Task> = match data.state() {
TaskState::Processing => get_tasks_by_state(
api.executing_tasks.tasks.get(&(data.namespace.clone(), data.task.clone())),
&data.namespace, &data.task
),
TaskState::Queued => get_tasks_by_state(
api.task_queue.tasks.get(&(data.namespace.clone(), data.task.clone())),
&data.namespace, &data.task
),
TaskState::Solved => get_tasks_by_state(
api.solved_tasks.tasks.get(&(data.namespace.clone(), data.task.clone())),
&data.namespace, &data.task
),
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/bin/server.rs between lines 50 and 135, the match arms for
TaskState::Processing, TaskState::Queued, and TaskState::Solved contain nearly
identical code that differs only in which task collection is accessed. To fix
this, extract the common logic into a helper function or closure that takes the
relevant task collection as input and returns the sorted and mapped
Vec<proto::Task>. Then, in the match arms, simply call this helper with the
appropriate collection, reducing duplication and improving maintainability.
Docstrings generation was requested by @IGN-Styly. * #27 (comment) The following files were modified: * `src/bin/server.rs`
|
Note Generated docstrings for this pull request at #28 |
Summary by CodeRabbit