Sharding keys on one range partition into on core#249
Conversation
update mtr test case update subm update mtr test result update subm based mar2 update mtr test update subm
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughUpdates the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Tip Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
storage/eloq/mysql-test/mono_main/r/compress.result (1)
141-144: Normalize the estimate-only output here as well.This file is asserting the same volatile
EXPLAIN.rowsandSHOW KEYS.Cardinalityvalues asselect.result. Since those numbers are planner/statistics artifacts rather than stable behavior, updating them in-place makes the compressed variant equally brittle to non-semantic engine changes.Example MTR normalization
--replace_column 9 # explain select fld3 from t2 ignore index (fld3) where fld3 = 'honeysuckle'; --replace_column 9 # explain select fld3 from t2 where fld1=fld1; --replace_column 9 # explain select min(fld1),max(fld1),count(*) from t2; --replace_column 7 # show keys from t2;Also applies to: 1267-1312, 1321-1321, 1375-1375, 1616-1616, 1668-1668, 1748-1750
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/eloq/mysql-test/mono_main/r/compress.result` around lines 141 - 144, The compressed test output records volatile planner/statistics fields (EXPLAIN.rows and SHOW KEYS.Cardinality) that must be normalized; update storage/eloq/mysql-test/mono_main/r/compress.result by adding MTR normalization directives (e.g. --replace_column <col> #) for the EXPLAIN and SHOW KEYS blocks referenced so the 9th column of EXPLAIN output and the Cardinality column in SHOW KEYS are replaced with a stable marker; apply the same normalization to the other mentioned ranges (1267-1312, 1321, 1375, 1616, 1668, 1748-1750) and to the EXPLAIN lines around the queries like "explain select fld3 from t2 ..." and "show keys from t2" so the file no longer asserts planner-estimated numbers.storage/eloq/mysql-test/mono_main/r/select.result (1)
138-141: Avoid hard-coding volatile optimizer estimates in this result file.These hunks only re-record
EXPLAIN.rows/SHOW KEYS.Cardinalityvalues (1198/1200). The suite already treats other unstable row estimates as placeholders, so pinning exact numbers here will keep causing result-only churn on harmless stats or planner changes. Please normalize those columns in the paired MTR test and keep the result focused on stable plan shape.Example MTR normalization
--replace_column 9 # explain select fld3 from t2 ignore index (fld3) where fld3 = 'honeysuckle'; --replace_column 9 # explain select fld3 from t2 where fld1=fld1; --replace_column 9 # explain select min(fld1),max(fld1),count(*) from t2; --replace_column 7 # show keys from t2;Also applies to: 550-550, 1260-1301, 1313-1313, 1366-1366, 1607-1607, 1659-1659, 1717-1719
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/eloq/mysql-test/mono_main/r/select.result` around lines 138 - 141, The result file hard-codes volatile optimizer estimates (EXPLAIN.rows and SHOW KEYS Cardinality like "1198"/"1200"); update the paired MTR test to normalize those columns instead of recording exact numbers: add --replace_column 9 # for the EXPLAIN lines (e.g. the EXPLAIN for "explain select fld3 from t2 use index (fld1) where fld3 = 'honeysuckle';" and other EXPLAINs) and add --replace_column 7 # for SHOW KEYS output (e.g. "show keys from t2;"), so the test compares stable plan shape not fluctuating row/cardinality estimates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@storage/eloq/mysql-test/mono_main/t/subselect_sj.test`:
- Around line 2909-2910: Replace the time-based workaround (--sleep 3) with a
deterministic bounded-retry cleanup around the DROP statement: detect the
transient deadlock/lock condition (e.g., check for active split-range/locks or
failed DROP error code) and retry the DROP in a small loop with exponential
backoff and a max attempt limit, failing the test only after the limit; update
the FIXME comment to include the tracked issue ID so it remains linked to the
bug. Locate the cleanup/drop logic in this test (the DROP TABLE / cleanup
sequence adjacent to the sleep) and implement the retry/wait there, ensuring you
only retry on the specific transient error conditions and not on permanent
failures.
---
Nitpick comments:
In `@storage/eloq/mysql-test/mono_main/r/compress.result`:
- Around line 141-144: The compressed test output records volatile
planner/statistics fields (EXPLAIN.rows and SHOW KEYS.Cardinality) that must be
normalized; update storage/eloq/mysql-test/mono_main/r/compress.result by adding
MTR normalization directives (e.g. --replace_column <col> #) for the EXPLAIN and
SHOW KEYS blocks referenced so the 9th column of EXPLAIN output and the
Cardinality column in SHOW KEYS are replaced with a stable marker; apply the
same normalization to the other mentioned ranges (1267-1312, 1321, 1375, 1616,
1668, 1748-1750) and to the EXPLAIN lines around the queries like "explain
select fld3 from t2 ..." and "show keys from t2" so the file no longer asserts
planner-estimated numbers.
In `@storage/eloq/mysql-test/mono_main/r/select.result`:
- Around line 138-141: The result file hard-codes volatile optimizer estimates
(EXPLAIN.rows and SHOW KEYS Cardinality like "1198"/"1200"); update the paired
MTR test to normalize those columns instead of recording exact numbers: add
--replace_column 9 # for the EXPLAIN lines (e.g. the EXPLAIN for "explain select
fld3 from t2 use index (fld1) where fld3 = 'honeysuckle';" and other EXPLAINs)
and add --replace_column 7 # for SHOW KEYS output (e.g. "show keys from t2;"),
so the test compares stable plan shape not fluctuating row/cardinality
estimates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82b7469c-d6b3-4678-8d06-08ac1da350ad
📒 Files selected for processing (8)
data_substratestorage/eloq/mysql-test/mono_main/r/compress.resultstorage/eloq/mysql-test/mono_main/r/select.resultstorage/eloq/mysql-test/mono_main/r/select_jcl6.resultstorage/eloq/mysql-test/mono_main/r/select_pkeycache.resultstorage/eloq/mysql-test/mono_main/r/ssl.resultstorage/eloq/mysql-test/mono_main/r/ssl_compress.resultstorage/eloq/mysql-test/mono_main/t/subselect_sj.test
| # FIXME: Temporary. To avoid deadlock caused by split range and drop table. | ||
| --sleep 3 |
There was a problem hiding this comment.
Replace time-based deadlock workaround with deterministic synchronization/retry
At Line 2910, --sleep 3 is timing-dependent and can still be flaky across environments. It also masks the underlying deadlock condition instead of making cleanup robust. Please switch this to a deterministic cleanup strategy (bounded retry/wait condition around the drop path) and keep the FIXME tied to a tracked issue ID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storage/eloq/mysql-test/mono_main/t/subselect_sj.test` around lines 2909 -
2910, Replace the time-based workaround (--sleep 3) with a deterministic
bounded-retry cleanup around the DROP statement: detect the transient
deadlock/lock condition (e.g., check for active split-range/locks or failed DROP
error code) and retry the DROP in a small loop with exponential backoff and a
max attempt limit, failing the test only after the limit; update the FIXME
comment to include the tracked issue ID so it remains linked to the bug. Locate
the cleanup/drop logic in this test (the DROP TABLE / cleanup sequence adjacent
to the sleep) and implement the retry/wait there, ensuring you only retry on the
specific transient error conditions and not on permanent failures.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
data_substrate (1)
1-1: Document the submodule update in the PR description.The submodule pointer update is valid. However, the recent commit history shows this repository follows a practice of including descriptive commit messages for submodule updates (e.g., "eloqstore bug fixes", "extend log retention"). Adding a brief description of what functional changes are included in the new
data_substratecommit would improve traceability for future debugging and be consistent with established patterns in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_substrate` at line 1, Add a short descriptive note about the submodule pointer update for data_substrate to the PR description: summarize the key functional changes introduced by the new data_substrate commit (e.g., bug fixes, API changes, retention/behavior tweaks), mention the new commit hash and any impacted areas or migration steps, and follow the established style used in prior messages like "eloqstore bug fixes" or "extend log retention" so the PR description and commit history remain traceable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@data_substrate`:
- Line 1: Add a short descriptive note about the submodule pointer update for
data_substrate to the PR description: summarize the key functional changes
introduced by the new data_substrate commit (e.g., bug fixes, API changes,
retention/behavior tweaks), mention the new commit hash and any impacted areas
or migration steps, and follow the established style used in prior messages like
"eloqstore bug fixes" or "extend log retention" so the PR description and commit
history remain traceable.
2 participants
Update mtr test case
Update submodule
Summary by CodeRabbit
Tests
Chores