1512: minor: add datafusion version info - #8
Conversation
Summary of ChangesHello, 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 enhances the observability and information reporting of Ballista components by integrating and exposing the underlying DataFusion version. This change allows for easier debugging and system monitoring by providing crucial version details directly in logs and API responses, without altering core functionality. Highlights
Changelog
Activity
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
|
WalkthroughThe pull request introduces DataFusion version tracking across multiple Ballista components. Four files are modified to import the DATAFUSION_VERSION constant and integrate it into system outputs. In the executor and scheduler processes, startup log messages are updated to display both Ballista and DataFusion versions. In the scheduler API handlers, a new datafusion_version field is added to the SchedulerStateResponse and SchedulerVersionResponse structures, which are populated with the DataFusion version during response construction. These changes maintain backward compatibility by not altering any exported API signatures. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the DataFusion version to the startup log messages for the Ballista executor and scheduler. It also extends the scheduler's API to include the DataFusion version in the state and version responses. The changes are consistent and improve the observability of the system by providing more detailed version information. The code modifications are correct and well-implemented.
🤖 Augment PR SummarySummary: Adds DataFusion version reporting alongside Ballista version in executor/scheduler startup logs and the scheduler REST API responses (/api/state, /api/version). 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
PR Review: Add DataFusion Version InfoOverall this is a clean, minimal, and useful change that improves observability by surfacing the DataFusion version at startup and via the API. Low risk, backward-compatible. A few observations below. Observations1. Import duplication across 4 files (minor)
pub use datafusion::DATAFUSION_VERSION;Then all four files could import from a single source ( 2. Silently dropped "Rust" qualifier in Old: The "Rust" qualifier is dropped without mention in the PR description. It originally distinguished from an older Java scheduler. Worth confirming this removal is intentional. 3. Mergeable imports in Two separate 4. API change is additive — no breaking change Adding 5. Test coverage No tests added or updated. Risk is low since these are startup messages and informational fields, but existing integration tests for Summary
The import consolidation via a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ballista/scheduler/src/api/handlers.rs (1)
99-130: Consider adding handler-level contract tests for the new field.A small JSON response assertion for
get_scheduler_stateandget_scheduler_versionwould prevent accidental regressions ofdatafusion_version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballista/scheduler/src/api/handlers.rs` around lines 99 - 130, Add handler-level contract tests that call get_scheduler_state and get_scheduler_version and assert the JSON responses include the new datafusion_version field (and other important fields like version/started/scheduling_policy) to prevent regressions; specifically, write tests that invoke get_scheduler_state (using SchedulerServer::state or a test double) and deserialize the Json response into SchedulerStateResponse to assert datafusion_version equals DATAFUSION_VERSION and similarly call get_scheduler_version and assert SchedulerVersionResponse.datafusion_version equals DATAFUSION_VERSION, ensuring the test covers cfg-dependent flags as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ballista/scheduler/src/api/handlers.rs`:
- Around line 99-130: Add handler-level contract tests that call
get_scheduler_state and get_scheduler_version and assert the JSON responses
include the new datafusion_version field (and other important fields like
version/started/scheduling_policy) to prevent regressions; specifically, write
tests that invoke get_scheduler_state (using SchedulerServer::state or a test
double) and deserialize the Json response into SchedulerStateResponse to assert
datafusion_version equals DATAFUSION_VERSION and similarly call
get_scheduler_version and assert SchedulerVersionResponse.datafusion_version
equals DATAFUSION_VERSION, ensuring the test covers cfg-dependent flags as
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a0c4df0-b636-4a1d-9813-64d66e67b6f6
📒 Files selected for processing (4)
ballista/executor/src/executor_process.rsballista/scheduler/src/api/handlers.rsballista/scheduler/src/scheduler_process.rsballista/scheduler/src/standalone.rs
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The Ballista project has no unit/IT tests for the REST API based on Axum. It would be good to start adding such! They would help preventing regressions in the future. |
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! The Ballista project has no unit/IT tests for the REST API based on Axum. It would be good to start adding such! They would help preventing regressions in the future. |
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! By using a re-export it will be much easier to update the path of DATAFUSION_VERSION if it ever changes in DataFusion itself. Currently it would have to be updated in 4 places. If re-export is used then it will be just one place. |
2 participants
1512: To review by AI