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.

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

Support MariaDB Native Driver #2787

Merged
merged 15 commits into from
Dec 26, 2022
Merged

Support MariaDB Native Driver #2787

merged 15 commits into from
Dec 26, 2022

Conversation

codychau
Copy link
Contributor

@codychau codychau commented Nov 27, 2022 •
edited by JohnNiang
Loading

What type of PR is this?

/kind improvement

What this PR does / why we need it:

引入对R2DBC的MariaDB的支持，增加例子配置

Special notes for your reviewer:

现在，Halo可以使用MariaDB数据库原生驱动了，摆脱MySQL驱动带来的连接问题。

  • 支持首次启动执行SQL脚本创建表

Does this PR introduce a user-facing change?

支持 MariaDB 数据库连接

@f2c-ci-robot f2c-ci-robot bot added release-note-none Denotes a PR that doesn't merit a release note. kind/improvement Categorizes issue or PR as related to a improvement. labels Nov 27, 2022
@CLAassistant
Copy link

CLAassistant commented Nov 27, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@codychau
Copy link
Contributor Author

你们手工测一下吧，我测过功能是正常才提交的。THX

@ruibaby
Copy link
Member

ruibaby commented Nov 27, 2022

@codychau 感谢你的贡献，在合并之前需要签署一下我们的 CLA #2787 (comment)

另外，似乎单元测试没有通过。

JohnNiang
JohnNiang reviewed Nov 27, 2022
View reviewed changes
build.gradle Outdated
@@ -92,6 +92,8 @@ dependencies {
// for more.
runtimeOnly 'io.r2dbc:r2dbc-h2'
runtimeOnly 'com.github.jasync-sql:jasync-r2dbc-mysql:2.1.7'
runtimeOnly 'org.mariadb:r2dbc-mariadb:1.1.2'
Copy link
Member

@JohnNiang JohnNiang Nov 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

需要注意的是，最新的 MariaDB（1.1.2）驱动并未实现 R2DBC Spec 1.0.0.RELEASE，目前仅支持 0.9.1.RELEASE，可能会存在不兼容的问题。这也是我们一直没有添加 Maria DB 驱动支持的缘故。

参考：

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的，明白了。感谢指导！不过我会身先士卒，自己先把所有的Halo的特性在MariaDB中充分测试，如果没有什么问题的话，可以尽快推出兼容方案，因为MariaDB仍是大势所趋，特别是云服务方面。如果因为不完全兼容R2DBC而导致的市场流失，损失可能更大。

Copy link
Member

Choose a reason for hiding this comment

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

@codychau 如果能够推进 MariaDB R2DBC connector 支持 R2DBC SPI 1.0.0.RELEASE 的话，可能会更好 : P

/meow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我觉得可以，但是我估计有很多人在做了，很快就会有相应的更新；说不定我们可以fork他们那一份，做一个尊享版Halo MariaDB Driver。更安全可靠。（开个玩笑）

Copy link
Member

Choose a reason for hiding this comment

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

@codychau 可以参考一下这个提交：mariadb-corporation/mariadb-connector-r2dbc@7a89c0d。他们似乎已经在 develop 分支支持了，等待他们发布最新的版本吧。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JohnNiang
或许我可以先尝试一下它们的开发分支，其实上面那个问题它们已经解决了，只是待其他功能一起发布而已，捂脸哭
image
你看嘛，1.1.3-快照版就支持了。解决方法也是简单粗暴！
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codychau 可以参考一下这个提交：mariadb-corporation/mariadb-connector-r2dbc@7a89c0d。他们似乎已经在 develop 分支支持了，等待他们发布最新的版本吧。

对的，这个是它们修复了跟R2DBC返回值长度不一致导致的问题，这个其实我也发现了，在处理一些ddl的时候会碰到

@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Nov 28, 2022

@JohnNiang: cat image

In response to this:

@codychau 如果能够推进 MariaDB R2DBC connector 支持 R2DBC SPI 1.0.0.RELEASE 的话，可能会更好 : P

/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@JohnNiang
Copy link
Member

Hi @codychau ，如果现在能够引入 org.mariadb:r2dbc-mariadb:1.1.3-SNAPSHOT 依赖，我们可以提前完成支持 MariaDB 这件事情，距离 2.1.0 发布还有一段时间，有足够的时间给到社区测试。

@f2c-ci-robot f2c-ci-robot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 7, 2022
@codychau
Copy link
Contributor Author

@JohnNiang 我最近在尝试用CI工具编译org.mariadb:r2dbc-mariadb:1.1.3-SNAPSHOT ，目前看是可以正常编译的，然后计划近期就以SystemScope的方式引入测试，如果都没什么问题的话，就会通知您

@JohnNiang
Copy link
Member

https://github.com/mariadb-corporation/mariadb-connector-r2dbc/releases/tag/1.1.3

mariadb connector r2dbc 终于发布新版了。

@codychau
Copy link
Contributor Author

@JohnNiang 我已经用上了，跟SQL相关的报错似乎都得到了解决，测试了两天后台日志都是比较的干净，功能也是正常的，不过我之前用1.1.2的时候功能貌似也没太大影响。现在有新的包肯定是用新的更好

1 similar comment
@codychau
Copy link
Contributor Author

@JohnNiang 我已经用上了，跟SQL相关的报错似乎都得到了解决，测试了两天后台日志都是比较的干净，功能也是正常的，不过我之前用1.1.2的时候功能貌似也没太大影响。现在有新的包肯定是用新的更好

@codychau codychau closed this Dec 24, 2022
@ruibaby
Copy link
Member

ruibaby commented Dec 24, 2022

@codychau 为什么要关闭这个 PR 然后再重新提交呢？ 这样会丢失所有 Review 记录。

@codychau
Copy link
Contributor Author

@ruibaby 我换了一个分支，之前我的分支加了很多自己的东西进去，用新的分支提交

@ruibaby
Copy link
Member

ruibaby commented Dec 25, 2022

@ruibaby 我换了一个分支，之前我的分支加了很多自己的东西进去，用新的分支提交

你可以参考 #3038 (comment) 操作一下，我建议使用还是使用这个 PR。

@codychau codychau reopened this Dec 26, 2022
@codychau
Copy link
Contributor Author

@ruibaby 我已经把我的main分支rebase了，提示“已经是最新”，无奈之下我把专门为了提PR的内容也给reset到了main分支，并且重开这个PR了，也检查了对比文件确认只剩我需要提交给你们的部分了。那个新PR我也给关闭了重启这个PR，已经仁至义尽了。

@JohnNiang JohnNiang changed the title Halo2.0 Support MariaDB Native Driver Support MariaDB Native Driver Dec 26, 2022
ruibaby
ruibaby reviewed Dec 26, 2022
View reviewed changes
Copy link
Member

@ruibaby ruibaby left a comment

Choose a reason for hiding this comment

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

/lgtm

image

image

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label Dec 26, 2022
JohnNiang
JohnNiang approved these changes Dec 26, 2022
View reviewed changes
Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

/approve

Thank you for your first contribution!

@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Dec 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JohnNiang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 26, 2022
@f2c-ci-robot f2c-ci-robot bot merged commit fcfc711 into halo-dev:main Dec 26, 2022
@ruibaby ruibaby mentioned this pull request Dec 26, 2022
Closed
@codychau
Copy link
Contributor Author

感谢各位的付出和包容，虽然最终结果是好的，但是给你们添了不少麻烦！给你们点个赞，辛苦了！下面是我的个人官网，目前仍在装修中，所使用的技术正是Halo2+MariaDB的环境，目前运行良好，我很喜欢，后面我会继续关注网友提出的问题并尝试修复，希望以后合作愉快！
http://nacgame.com/upassistant2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruibaby ruibaby ruibaby left review comments

@JohnNiang JohnNiang JohnNiang approved these changes

@lan-yonghui lan-yonghui Awaiting requested review from lan-yonghui

Assignees

@ruibaby ruibaby

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/improvement Categorizes issue or PR as related to a improvement. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@codychau @CLAassistant @ruibaby @JohnNiang