Skip to content

Remove redundant overriding tests in Kudu#12952

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
chen-ni:issue-11815-part-3
Jul 1, 2022
Merged

Remove redundant overriding tests in Kudu#12952
ebyhr merged 1 commit intotrinodb:masterfrom
chen-ni:issue-11815-part-3

Conversation

@chen-ni
Copy link
Copy Markdown
Contributor

@chen-ni chen-ni commented Jun 23, 2022

Description

Is this change a fix, improvement, new feature, refactoring, or other?

An improvement in testing.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Connector tests.

How would you describe this change to a non-technical end-user or system administrator?

Improved some tests that shouldn't change any behavior.

Related issues, pull requests, and links

This is the third one of a series of PRs to implement #11815. It fixes 5 of the 8 remaining tests.

The 5 tests are removed because the overridden tests are working now as #12915 fixed the issue.

Documentation

(x) No documentation is needed.

Release notes

(x) No release notes entries required.

@cla-bot cla-bot bot added the cla-signed label Jun 23, 2022
@chen-ni chen-ni marked this pull request as ready for review June 23, 2022 07:58
@chen-ni chen-ni requested a review from ebyhr June 23, 2022 07:58
@chen-ni chen-ni marked this pull request as draft June 23, 2022 09:14
@chen-ni chen-ni removed the request for review from ebyhr June 23, 2022 09:14
@chen-ni chen-ni marked this pull request as ready for review June 24, 2022 02:32
@chen-ni
Copy link
Copy Markdown
Contributor Author

chen-ni commented Jun 24, 2022

@ebyhr By the way, sometimes I got this error (2 out of the 6 Kudu connector tests in my most recent run) while executing testReadMetadataWithRelationsConcurrentModifications (which leads to failing the test):

java.util.concurrent.ExecutionException: java.lang.RuntimeException: java.lang.RuntimeException: org.apache.kudu.client.NonRecoverableException: table presto::tpch.concur_table_54frruwm4s was deleted: Table deleted at 2022-06-24 01:49:33 UTC

	at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)
	at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191)
	at io.trino.testing.BaseConnectorTest.testReadMetadataWithRelationsConcurrentModifications(BaseConnectorTest.java:1442)
	at io.trino.testing.BaseConnectorTest.testReadMetadataWithRelationsConcurrentModifications(BaseConnectorTest.java:1381)
	...
Caused by: java.lang.RuntimeException: java.lang.RuntimeException: org.apache.kudu.client.NonRecoverableException: table presto::tpch.concur_table_54frruwm4s was deleted: Table deleted at 2022-06-24 01:49:33 UTC
	at io.trino.testing.AbstractTestingTrinoClient.execute(AbstractTestingTrinoClient.java:122)
	at io.trino.testing.DistributedQueryRunner.execute(DistributedQueryRunner.java:526)
	at io.trino.testing.AbstractTestQueryFramework.computeActual(AbstractTestQueryFramework.java:217)
	at io.trino.testing.AbstractTestQueryFramework.computeActual(
	...
	Suppressed: java.lang.Exception: SQL: SELECT * FROM system.jdbc.columns WHERE table_cat = CURRENT_CATALOG AND table_schem = CURRENT_SCHEMA
		at io.trino.testing.DistributedQueryRunner.execute(DistributedQueryRunner.java:529)
		... 11 more
	Suppressed: java.lang.Exception: Task: Query(SELECT * FROM system.jdbc.columns WHERE table_cat = CURRENT_CATALOG AND table_schem = CURRENT_SCHEMA)
		at io.trino.testing.BaseConnectorTest$3.call(BaseConnectorTest.java:1539)
		... 6 more

Is it normal?

@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jun 24, 2022

It isn't normal. Could you file an issue and fix the test likes BaseSqlServerConnectorTest#testReadMetadataWithRelationsConcurrentModifications ?

@chen-ni
Copy link
Copy Markdown
Contributor Author

chen-ni commented Jun 25, 2022

@ebyhr Please see my update: https://github.com/trinodb/trino/pull/12952/files#diff-97b52ff46b1f77c92e023e1f434bbe2284a2b0e0918f146e7ca11b6e969e9c24R507 :)

It works on my local machine, passing all the tests. But I'm not sure why the GitHub check was canceled:https://github.com/trinodb/trino/runs/7041780965?check_suite_focus=true

Should we retrigger the check?

@chen-ni chen-ni requested a review from ebyhr June 25, 2022 01:07
@chen-ni
Copy link
Copy Markdown
Contributor Author

chen-ni commented Jun 26, 2022

@ebyhr Should we split the 2 commits into 2 separate PRs to verify which one is causing the CI to fail? (Seems ci / test (plugin/trino-kudu) (pull_request) took too long and was canceled.)

@chen-ni chen-ni changed the title Implement 8 tests in Kudu connector tests Remove redundant overriding tests in Kudu Jun 27, 2022
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jun 27, 2022

@chen-ni Just triggered failed CI. Do you want to fix #12576?

@chen-ni
Copy link
Copy Markdown
Contributor Author

chen-ni commented Jun 27, 2022

@ebyhr Yes, I'd love to fix it :)

@chen-ni
Copy link
Copy Markdown
Contributor Author

chen-ni commented Jun 30, 2022

@ebyhr The checks pass now, I think it's ready to be merged :)

@ebyhr ebyhr merged commit 746eb83 into trinodb:master Jul 1, 2022
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jul 1, 2022

Merged, thanks!

@github-actions github-actions bot added this to the 389 milestone Jul 1, 2022
@chen-ni chen-ni deleted the issue-11815-part-3 branch July 1, 2022 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants