Skip to content

feat(spider-storage): Add support for MySQL database. - #450

Merged
sitaowang1998 merged 1 commit into
y-scope:mainfrom
sitaowang1998:mysql-support
Aug 21, 2026
Merged

feat(spider-storage): Add support for MySQL database.#450
sitaowang1998 merged 1 commit into
y-scope:mainfrom
sitaowang1998:mysql-support

Conversation

@sitaowang1998

@sitaowang1998 sitaowang1998 commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Description

Background

The current Spider only support MariaDB as the database in storage, and it uses RETURNING ... syntax, which is a MariaDB-only extension.

During @20001020ycx testing, the incompatibility with MySQL database was exposed.

Change

This PR replaces all 5 RETURNING id in the code with sqlx::last_insert_id. Now Spider supports MySQL as database.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Unit Tests

Run unit tests with both MariaDB and MySQL 8.0-debian. All tests pass.

End-to-End Tests

Run the end-to-end tests from #385 using Docker Compose local support. The end-to-end tests run on two clusters, one using the bundled MariaDB, the other one with bundled MariaDB replaced by MySQL 8.0-debian. Both test executions pass.

Summary by CodeRabbit

  • Bug Fixes
    • Improved MariaDB compatibility for creating sessions, jobs, resource groups, execution managers, and scheduler registrations.
    • Generated identifiers are now retrieved reliably after insert operations, with existing error handling and transaction behaviour preserved.

@sitaowang1998
sitaowang1998 requested a review from a team as a code owner August 21, 2026 19:10
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 30c3ac1c-2b9b-464b-8aa9-7abbab23ce90

📥 Commits

Reviewing files that changed from the base of the PR and between 5a78612 and abaa9ac.

📒 Files selected for processing (1)
  • components/spider-storage/src/db/mariadb.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

Changes

MariaDB generated ID handling

Layer / File(s) Summary
Replace RETURNING with last_insert_id()
components/spider-storage/src/db/mariadb.rs
Session, job, resource-group, execution-manager, and scheduler inserts now execute without RETURNING. Generated identifiers come from last_insert_id() and retain existing type conversion, transaction handling, and error mapping.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to abaa9

The change adds MySQL storage support by replacing MariaDB-specific insert-returning behavior, with reported end-to-end tests passing for both MariaDB and MySQL; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: linzhihao-723

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: adding MySQL database support to Spider storage.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@LinZhihao-723 LinZhihao-723 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, storage is not a valid scope for the PR title/commit message. Please spend more time drafting the PR title, as you are constantly making this mistake.

@sitaowang1998 sitaowang1998 changed the title feat(storage): Add support for MySQL database. feat(spider-storage): Add support for MySQL database. Aug 21, 2026

@LinZhihao-723 LinZhihao-723 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we merge: can you run unit tests with MySQL DB backend to make sure task test:rust-unit-tests can pass?

@sitaowang1998

Copy link
Copy Markdown
Collaborator Author

@CodeRabbit Please open an issue to run unit tests on both MariaDB and MySQL in task and in CI.

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="y-scope/spider"
pr_number="450"
requester="sitaowang1998"
pr_url="https://github.com/${repo}/pull/${pr_number}"

comment_url="$(
  gh api "repos/${repo}/issues/${pr_number}/comments" --paginate \
    --jq '.[] | select(.user.login == "sitaowang1998" and (.body | contains("Please open an issue to run unit tests on both MariaDB and MySQL in task and in CI."))) | .html_url' \
    | head -n 1
)"

if [ -z "${comment_url}" ]; then
  comment_url="${pr_url}"
fi

issue_body="$(cat <<EOF
## Summary

Run the storage unit-test suite against both MariaDB and MySQL.

## Rationale

PR `#450` removes MariaDB-specific \`RETURNING\` syntax so Spider supports MySQL. The test workflow must validate database behaviour on both supported database engines.

## Required changes

- Update the task-based local test workflow to run the relevant unit tests with MariaDB and with MySQL.
- Update CI to run the relevant unit tests with MariaDB and with MySQL.
- Keep database setup and test configuration explicit for each engine.
- Ensure test failures identify the database engine that failed.

## Affected areas

- Storage database test configuration.
- Task definitions for unit tests.
- CI workflow definitions that provision database services and run unit tests.

## Acceptance criteria

- A developer can run the relevant unit tests through the project task workflow against MariaDB.
- A developer can run the same unit tests through the project task workflow against MySQL.
- CI executes the relevant unit tests against MariaDB and MySQL.
- CI reports failures separately for each database engine.
- The documented test commands and CI configuration remain reproducible.

## Backlinks

- Pull request: ${pr_url}
- Request comment: ${comment_url}
- Requested by: @${requester}
EOF
)"

issue_url="$(
  gh issue create \
    --repo "${repo}" \
    --title "test(storage): Run unit tests against MariaDB and MySQL in task workflows and CI" \
    --body "${issue_body}" \
    --assignee "${requester}"
)"

printf 'Created issue: %s\n' "${issue_url}"

Length of output: 2267


@sitaowang1998 Created #451: test(storage): Run unit tests against MariaDB and MySQL in task workflows and CI. It is assigned to you.

You are interacting with an AI system.

@sitaowang1998
sitaowang1998 merged commit b1cfd3c into y-scope:main Aug 21, 2026
26 checks passed
@sitaowang1998
sitaowang1998 deleted the mysql-support branch August 21, 2026 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants