-
Notifications
You must be signed in to change notification settings - Fork 5k
fix: explain no response issue #34435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @dapan1121, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily addresses an issue where 'explain' operations might not correctly handle responses, leading to a 'no response' state. It refactors how job contexts and notifications are managed within the scheduler, particularly for parent-child job relationships and explain queries. By introducing a new macro for parent job identification and enhancing task notification logic, the changes ensure proper completion and cleanup of jobs, thereby improving the overall robustness and reliability of the scheduler's handling of complex query types. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to fix an issue where 'explain' queries do not receive a response, likely in scenarios involving sub-queries. The changes introduce helper macros to get the parent job and its explain context, and uses them to centralize explain-related operations. Additionally, task notification logic is updated to handle sub-jobs and allow for null tasks. While most of the changes seem correct, I've found a critical bug in schJob.c where sub-job tasks are not being notified correctly due to a potential copy-paste error. Please see the specific comment for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adjusts scheduler handling for EXPLAIN jobs (especially with sub-jobs) and tweaks a CI task list, aiming to fix a “no response” issue during EXPLAIN executions.
Changes:
- Guard
schNotifyTaskInHashListagainst aNULLcurrent task and extendschNotifyJobAllTasksto propagate notifications to sub-jobs. - Introduce
SCH_PARENT_JOBand consistently useSCH_JOB_EXPLAIN_CTX/SCH_PARENT_JOBin scheduler remote response handling so EXPLAIN state is tracked on the parent job. - Update several entries in
test/ci/cases.taskfor subquery-related tests to use barepytestinstead of the./ci/pytest.shwrapper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/ci/cases.task | Adjusts how a set of subquery and related tests are invoked in the CI task matrix. |
| source/libs/scheduler/src/schTask.c | Safely skips direct notification when pCurrTask is NULL before iterating the task hash list. |
| source/libs/scheduler/src/schRemote.c | Routes EXPLAIN context and completion handling through the parent job to ensure results and notifications are attached to the correct job. |
| source/libs/scheduler/src/schJob.c | Extends job-wide task notification to cover sub-jobs, but currently misuses the parent’s task list inside the sub-job loop. |
| source/libs/scheduler/inc/schInt.h | Adds SCH_PARENT_JOB macro and centralizes access to the appropriate EXPLAIN context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.