8000 fix bug: dot not upgrade BM25 by ouyuanning · Pull Request #21661 · matrixorigin/matrixone · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix bug: dot not upgrade BM25 #21661

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 8, 2025
Merged

fix bug: dot not upgrade BM25 #21661

merged 6 commits into from
Apr 8, 2025

Conversation

ouyuanning
Copy link
Contributor
@ouyuanning ouyuanning commented Apr 7, 2025 •
edited by qodo-merge-pro bot
Loading

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #21377

What this PR does / why we need it:

fix bub. dot not upgrade BM25

PR Type

Bug fix

Description

  • Added upgradeForBM25 function call in tenant upgrade process.

  • Logged errors for BM25 upgrade failures with relevant details.

  • Ensured BM25 upgrade is part of the tenant upgrade workflow.

Changes walkthrough 📝

Relevant files
Bug fix
upgrade.go
Integrate BM25 upgrade into tenant upgrade process             

pkg/bootstrap/versions/v2_1_0/upgrade.go

  • Added upgradeForBM25 function call during tenant upgrade.
  • Logged errors for BM25 upgrade failures.
  • Enhanced tenant upgrade process to include BM25 upgrades.
    		• +6/-0     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    qodo-merge-pro bot commented Apr 7, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Incorrect Log Message

    The error log message for BM25 upgrade uses upgEntry.String() which refers to the previous upgrade entry, not the BM25 upgrade. This may cause confusion when debugging as it doesn't accurately represent the failing component.

     
    getLogger(txn.Txn().TxnOptions().CN).Error("tenant upgrade entry execute for BM25 error", zap.Error(err), zap.Int32("tenantId", tenantID), zap.String("version", v.Metadata().Version), zap.String("upgrade for BM25", upgEntry.String()))
return err

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    qodo-merge-pro bot commented Apr 7, 2025 •
    edited
    Loading

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    ✅ Fix incorrect log identifier
    Suggestion Impact:The commit addressed the issue with the BM25 error log, but instead of replacing upgEntry.String() with 'BM25' as suggested, it completely removed that parameter from the error log. The commit also restructured the code to move the BM25 upgrade outside the loop.

    code diff:

    -		err = upgradeForBM25(uint32(tenantID), txn)
-		if err != nil {
-			getLogger(txn.Txn().TxnOptions().CN).Error("tenant upgrade entry execute for BM25 error", zap.Error(err), zap.Int32("tenantId", tenantID), zap.String("version", v.Metadata().Version), zap.String("upgrade for BM25", upgEntry.String()))
-			return err
-		}
-
 		duration := time.Since(start)
 		getLogger(txn.Txn().TxnOptions().CN).Info("tenant upgrade entry complete",
 			zap.String("upgrade entry", upgEntry.String()),
 			zap.Int64("time cost(ms)", duration.Milliseconds()),
 			zap.Int32("tenantId", tenantID),
 			zap.String("toVersion", v.Metadata().Version))
+	}
+
+	err := upgradeForBM25(uint32(tenantID), txn)
+	if err != nil {
+		getLogger(txn.Txn().TxnOptions().CN).Error("tenant upgrade entry execute for BM25 error", zap.Error(err), zap.Int32("tenantId", tenantID), zap.String("version", v.Metadata().Version))
+		return err

    The error log for BM25 upgrade incorrectly uses upgEntry.String() instead of a
    BM25-specific identifier. This will show the previous upgrade entry name rather
    than indicating it's a BM25 upgrade.

    pkg/bootstrap/versions/v2_1_0/upgrade.go [73-77]

     err = upgradeForBM25(uint32(tenantID), txn)
 if err != nil {
-    getLogger(txn.Txn().TxnOptions().CN).Error("tenant upgrade entry execute for BM25 error", zap.Error(err), zap.Int32("tenantId", tenantID), zap.String("version", v.Metadata().Version), zap.String("upgrade for BM25", upgEntry.String()))
+    getLogger(txn.Txn().TxnOptions().CN).Error("tenant upgrade entry execute for BM25 error", zap.Error(err), zap.Int32("tenantId", tenantID), zap.String("version", v.Metadata().Version), zap.String("upgrade type", "BM25"))
     return err
 }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that using upgEntry.String() in the BM25 error log is misleading as it refers to the previous upgrade entry rather than the BM25 upgrade. Replacing it with a static "BM25" identifier makes the logs clearer and more accurate for debugging purposes.

    		Medium
    • Update

    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Apr 7, 2025
    @mergify mergify bot added the kind/bug Something isn't working label Apr 7, 2025
    @mergify mergify bot merged commit 16211d9 into matrixorigin:main Apr 8, 2025
    18 checks passed
    @ouyuanning ouyuanning deleted the fix-bug-donot-upgrade-bm25 branch June 3, 2025 00:55
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @qingxinhome qingxinhome qingxinhome approved these changes

    @daviszhen daviszhen Awaiting requested review from daviszhen daviszhen is a code owner

    @zhangxu19830126 zhangxu19830126 Awaiting requested review from zhangxu19830126 zhangxu19830126 is a code owner

    @LeftHandCold LeftHandCold Awaiting requested review from LeftHandCold LeftHandCold is a code owner

    Assignees
    No one assigned
    Labels
    kind/bug Something isn't working Review effort 2/5 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    3 participants
    @ouyuanning @qingxinhome @matrix-meow
    0