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

[#5289][Improvement] CatalogMysqlDriverIT miss driver url #5431

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

lsyulong
Copy link
Contributor

@lsyulong lsyulong commented Nov 4, 2024

What changes were proposed in this pull request?

[#5289][Improvement] CatalogMysqlDriverIT miss driver url

Why are the changes needed?

[#5289][Improvement] CatalogMysqlDriverIT miss driver url

Fix: #5289

Does this PR introduce any user-facing change?

no

How was this patch tested?

no

@lsyulong
Copy link
Contributor Author

lsyulong commented Nov 5, 2024

@mchades Hello, I submitted a PR here and it has been successfully built. I see that there are still two integration tests that have not been successful. Do you need me to fix them as well

@mchades
Copy link
Contributor

mchades commented Nov 5, 2024

sure, you should resolve these issues to ensure the CI passes.

@jerryshao
Copy link
Contributor

@lsyulong https://github.com/apache/gravitino/actions/runs/11675764065/job/32524822188?pr=5431 this failure seems not a flaky CI problem, maybe due to your changes, can you please fix the tests?

@lsyulong
Copy link
Contributor Author

lsyulong commented Nov 5, 2024

@lsyulong https://github.com/apache/gravitino/actions/runs/11675764065/job/32524822188?pr=5431 this failure seems not a flaky CI problem, maybe due to your changes, can you please fix the tests?https://github.com/apache/gravitino/actions/runs/11675764065/job/32524822188?pr=5431这个失败似乎不是一个片状的 CI 问题,也许是由于你的更改,你能修复测试吗?

Okay, I'll take a look

@lsyulong
Copy link
Contributor Author

lsyulong commented Nov 6, 2024

image
@jerryshao I have added some debugging information here, but my local computer cannot debug as there is no corresponding environment. Can you help me run the CI and I will check the logs in the corresponding integration testing

@jerryshao jerryshao closed this Nov 6, 2024
@jerryshao jerryshao reopened this Nov 6, 2024
@lsyulong
Copy link
Contributor Author

lsyulong commented Nov 6, 2024

@jerryshao @mchades Two teachers, I submitted the code and found that the integration test failed to build again. Can you provide me with some help

@yuqi1129
Copy link
Contributor

yuqi1129 commented Nov 6, 2024

@lsyulong
Please take a look at the PR and the corresponding issue first. https://github.com/apache/gravitino/pull/3294/files
There are some small differences between different kinds of JDBC drivers in table/schema meta, please check it first if you still have problems, please ping us.

@lsyulong
Copy link
Contributor Author

lsyulong commented Nov 6, 2024

@lsyulong Please take a look at the PR and the corresponding issue first. https://github.com/apache/gravitino/pull/3294/files请先看一下 PR 和相应的 Issue。 https://github.com/apache/gravitino/pull/3294/files There are some small differences between different kinds of JDBC drivers in table/schema meta, please check it first if you still have problems, please ping us.不同类型的 JDBC 驱动程序在表/架构元数据中存在一些细微差异,请先检查一下,如果仍有问题,请 ping 我们。

ok, thanks

@lsyulong
Copy link
Contributor Author

lsyulong commented Nov 7, 2024

image
@yuqi1129 I saw the RP link you provided, and I replaced the MySQL address in my BaseIT class with 8.0.11 instead of 8.0.23. Is there anything else I couldn't consider? Could you please help me check and correct it

@yuqi1129
Copy link
Contributor

yuqi1129 commented Nov 7, 2024

image @yuqi1129 I saw the RP link you provided, and I replaced the MySQL address in my BaseIT class with 8.0.11 instead of 8.0.23. Is there anything else I couldn't consider? Could you please help me check and correct it

Let me clarify the original problem.

The version of the JDBC driver is 8.0.23 in BaseIT originally. Someone reported that something is wrong when using 8.0.11, and we have confirmed this issue. This problem was fixed by #3294 (files). However, in one or two versions, someone has removed the related test that uses 8.0.11, so we need to add them back. That's what you are doing.

If we set the version of the JDBC driver to 8.0.11 and there is an exception in the tests, I believe we need to fix those errors. If you need help, you can try and ping us.

@lsyulong
Copy link
Contributor Author

lsyulong commented Nov 7, 2024

image @yuqi1129 I saw the RP link you provided, and I replaced the MySQL address in my BaseIT class with 8.0.11 instead of 8.0.23. Is there anything else I couldn't consider? Could you please help me check and correct it

Let me clarify the original problem.

The version of the JDBC driver is 8.0.23 in BaseIT originally. Someone reported that something is wrong when using 8.0.11, and we have confirmed this issue. This problem was fixed by #3294 (files). However, in one or two versions, someone has removed the related test that uses 8.0.11, so we need to add them back. That's what you are doing.

If we set the version of the JDBC driver to 8.0.11 and there is an exception in the tests, I believe we need to fix those errors. If you need help, you can try and ping us.

@yuqi1129 OK, my understanding is to add back the missing content from the CatalogDoris Driver IT and CatalogMysql Driver IT classes. The rest of the code structure and logic have changed. Do we still need to add this part?
image

@yuqi1129
Copy link
Contributor

yuqi1129 commented Nov 7, 2024

image @yuqi1129 I saw the RP link you provided, and I replaced the MySQL address in my BaseIT class with 8.0.11 instead of 8.0.23. Is there anything else I couldn't consider? Could you please help me check and correct it

Let me clarify the original problem.
The version of the JDBC driver is 8.0.23 in BaseIT originally. Someone reported that something is wrong when using 8.0.11, and we have confirmed this issue. This problem was fixed by #3294 (files). However, in one or two versions, someone has removed the related test that uses 8.0.11, so we need to add them back. That's what you are doing.
If we set the version of the JDBC driver to 8.0.11 and there is an exception in the tests, I believe we need to fix those errors. If you need help, you can try and ping us.

@yuqi1129 OK, my understanding is to add back the missing content from the CatalogDoris Driver IT and CatalogMysql Driver IT classes. The rest of the code structure and logic have changed. Do we still need to add this part? image

I don't think so. The only thing you need to do is to set driver version to 8.0.11 in CatalogMysqlDriverIT.java and make all CI PASS.

@yuqi1129
Copy link
Contributor

yuqi1129 commented Nov 7, 2024

@lsyulong
Can you run the test locally? I suggest you do so locally as CI is very time-consuming and hard to debug.

@lsyulong
Copy link
Contributor Author

lsyulong commented Nov 7, 2024

@lsyulong Can you run the test locally? I suggest you do so locally as CI is very time-consuming and hard to debug.你可以在本地运行测试吗?我建议您在本地进行此操作,因为 CI 非常耗时且难以调试。

ok

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.

[Improvement] CatalogMysqlDriverIT miss driver url
4 participants