-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Fix][JDBC] Fix starrocks jdbc dialect catalog conflict with starrocks connector #7578
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test case for this?
94c3199
to
08d2e82
Compare
value = {}, | ||
type = {EngineType.FLINK, EngineType.SPARK}, | ||
disabledReason = "only Zeta Engine can get this log") | ||
void checkResult(String executeKey, TestContainer container, Container.ExecResult execResult) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
f2c6054
to
366b9a4
Compare
a5d4931
to
45e5ba4
Compare
@@ -112,7 +112,7 @@ public abstract class AbstractJdbcIT extends TestSuiteBase implements TestResour | |||
|
|||
abstract JdbcCase getJdbcCase(); | |||
|
|||
abstract void compareResult(String executeKey) throws SQLException, IOException; | |||
void checkResult(String executeKey, TestContainer container, Container.ExecResult execResult) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add more parameter to help check log.
and delete the abstract keyword, the implement class only need implement it when necessary.
if (container.identifier().equals(TestContainerId.SEATUNNEL)) { | ||
Assertions.assertTrue( | ||
execResult.getStdout().contains("Loading catalog tables for catalog")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i run some test, find the stdout and container log is different on different engine and version.
so here only check seatunnel engine's log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when has this log Loading catalog tables for catalog
means it was load table from MySQLCatalog. rather than
use jdbc.
related log location:
https://github.com/apache/seatunnel/blob/dev/seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java#L77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Purpose of this pull request
close #7551
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note
.