Skip to content
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.

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

Bugfix: DB path #146

Merged
merged 2 commits into from
Jan 15, 2025
Merged

Bugfix: DB path #146

merged 2 commits into from
Jan 15, 2025

Conversation

tunmx
Copy link
Member

@tunmx tunmx commented Jan 15, 2025

No description provided.

Copy link

devloai bot commented Jan 15, 2025

Automatic code review is enabled for this repository. Reviewing this PR now.

Copy link

@devloai devloai bot left a comment

Choose a reason for hiding this comment

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

The core DB path fix looks good, but there is a critical issue that needs to be addressed: the CI workflow file was accidentally modified to trigger on the wrong branch. This needs to be fixed by reverting back to "release/wheels". Additionally, consider adding warning logs for better error handling when persistence is enabled without a path.

@@ -2,9 +2,9 @@ name: Build Wheels

on:
push:
branches: ["release/wheels"]
branches: ["dev/patch-7"]
Copy link

Choose a reason for hiding this comment

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

The workflow trigger branch should not be changed from "release/wheels" to "dev/patch-7". This appears to be an unintended change that could break the wheel building process. Please keep it as "release/wheels".

Suggested change
branches: ["dev/patch-7"]
branches: ["release/wheels"]

@@ -386,11 +386,12 @@ HResult HFFeatureHubDataEnable(HFFeatureHubConfiguration configuration) {
if (configuration.enablePersistence) {
Copy link

Choose a reason for hiding this comment

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

The fix for handling persistenceDbPath looks good, but there's a missing warning log when persistenceDbPath is null and enablePersistence is true. This could help with debugging issues. Consider adding a warning log in this case.

@tunmx tunmx merged commit 3be1ef2 into master Jan 15, 2025
13 checks passed
@tunmx tunmx deleted the dev/patch-7 branch January 21, 2025 08:52
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.

1 participant