Refactor database configuration in 00_database.php#296
Conversation
Removed conditional database configuration and added a reference to basic DB configuration in LocalSettings.php. Fix #295
📝 WalkthroughWalkthroughMinor formatting-only edits in ChangesFormatting & Whitespace Adjustment
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
@copilot check if that solves the linked issue and double check that the dbhost is set in production |
There was a problem hiding this comment.
Pull request overview
This PR refactors MediaWiki database configuration by removing the conditional multi-server LBFactoryMulti configuration from LocalSettings.d/base/00_database.php and instead pointing to the “basic DB configuration” handled via LocalSettings.php (generated from LocalSettings.php.template). This aligns with the goal of avoiding the replica lag detection behavior described in issue #295.
Changes:
- Removed conditional
LBFactoryMulticonfiguration that depended onDB_PRIMARY_IP/DB_SECONDARY_IP. - Kept per-wiki
$wgDBnameselection logic and adjusted a comment (“English” capitalization). - Added an in-file reference to where the base DB credentials are configured.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ), | ||
| ); | ||
| } | ||
| // Basic db configuration in Localsettings.php https://github.com/MaRDI4NFDI/docker-wikibase/blob/7146f8b2fdbba680d11ffffb32b51d3961f50a9c/LocalSettings.php.template#L12-L16 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
LocalSettings.d/base/00_database.php (1)
31-35:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Code changes do not address the stated PR objectives.
The PR description claims to have "Removed conditional database configuration" to address issue
#295regarding database lag detection and Varnish cache TTL. However, the actual changes shown are:
- Line 31: Capitalization of a comment (cosmetic only)
- Line 34: Addition of a reference comment pointing to LocalSettings.php
There are no functional changes to database configuration, no removal of conditional logic, and no changes related to 'max lag' settings or replication lag detection mentioned in issue
#295.The comment on line 34 suggests the actual database configuration changes are in
LocalSettings.php, but that file is not included in this PR for review. This creates several problems:
- Incomplete PR: If the actual fix is in LocalSettings.php, that file should be included in this PR
- Misleading description: The PR title and description don't match the changes
- Unverifiable fix: Without seeing the changes to LocalSettings.php, we cannot verify that issue
#295is actually addressedAdditionally, line 34 has a typo: "Localsettings.php" should be "LocalSettings.php" (capital S).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@LocalSettings.d/base/00_database.php` around lines 31 - 35, The PR claims to remove conditional database configuration for replication lag detection but only contains cosmetic comment changes; update the branch to include the actual functional changes in LocalSettings.php (the DB config that sets replication max lag / removes conditional logic used for lag detection and Varnish TTL), ensure any conditional logic around $wgDBservers / replication/max lag is removed or replaced with the intended always-on configuration in the same PR so reviewers can verify fix, and fix the typo in the comment ("Localsettings.php" → "LocalSettings.php") so the referenced filename matches the actual file; reference the LocalSettings.php DB configuration block and any variables like $wgDBservers, $wgDBserver, $wgDBadminuser, and max lag settings when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@LocalSettings.d/base/00_database.php`:
- Around line 31-35: The PR claims to remove conditional database configuration
for replication lag detection but only contains cosmetic comment changes; update
the branch to include the actual functional changes in LocalSettings.php (the DB
config that sets replication max lag / removes conditional logic used for lag
detection and Varnish TTL), ensure any conditional logic around $wgDBservers /
replication/max lag is removed or replaced with the intended always-on
configuration in the same PR so reviewers can verify fix, and fix the typo in
the comment ("Localsettings.php" → "LocalSettings.php") so the referenced
filename matches the actual file; reference the LocalSettings.php DB
configuration block and any variables like $wgDBservers, $wgDBserver,
$wgDBadminuser, and max lag settings when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f701a7b-9b2a-42f7-a6b8-a05ea3fd2374
📒 Files selected for processing (1)
LocalSettings.d/base/00_database.php
Yes, this solves the linked issue. The root cause was the Regarding |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/MaRDI4NFDI/docker-wikibase/sessions/a050f1c1-3601-499a-9f5b-555b4f26e141 Co-authored-by: physikerwelt <2777736+physikerwelt@users.noreply.github.com>
Done in 0b33f57. Fixed the comment on line 34: corrected filename casing to |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@LocalSettings.d/base/00_database.php`:
- Line 34: The comment incorrectly implies $wgDBname is only set in
LocalSettings.php.template; update the comment to state that basic DB
configuration variables ($wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword) are
configured here and that $wgDBname is assigned/defaulted and may be overridden
conditionally within this file (refer to the $wgDBname assignments), so future
maintainers are not misled about the origin of $wgDBname.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c30e915-14e0-4b01-bd43-4ebeb354dfb7
📒 Files selected for processing (1)
LocalSettings.d/base/00_database.php
|
The database is currently overloaded, that is why this approach does not work |
|
WMF does not use Galera, and some versions are broken. https://phabricator.wikimedia.org/T338070 documents that they would rather document incompatibily then fixing things. In addition, our ongoing performance problems may indicate that the Galera experiment has failed and we should consider using the well-established methods that WMF is using. |
Removed conditional database configuration and added a reference to basic DB configuration in LocalSettings.php.
Fix #295
Summary by CodeRabbit