Upgrade kudu client to 1.15.0 by grantatspothero · Pull Request #10940 · trinodb/trino

grantatspothero · 2022-02-03T19:50:11Z

Upgrading the kudu client revealed a few problems:

Timeouts to kudu tablets were sometimes occurring during deletes due to this change introduced in the kudu java client in version 1.13.0: apache/kudu@d23ee5d#diff-f1f50409d81052b8f8d7aea7da663c185c704c6206cb0ec901114f4d9ee8c28f
(see here for the reason why that commit broke the client: https://gerrit.cloudera.org/#/c/18166/)
Timeouts were not failing query execution because the kudu connector was configured to flush operations in the background.
The two combined above meant tests that did deletes sometimes actually did not perform deletes and would fail.

The first commit upgrades the kudu client and additionally explicitly fails trino execution when kudu rpcs timeout. This resolves: #5687

The second commit fixes the problem of one source of kudu timeouts in the 1.15.0 client .

grantatspothero · 2022-02-03T19:52:08Z

Want to let the CI process run so there is a build showing the timeouts causing query failure.

After that, will push another commit to actually resolve the timeout problems by setting KuduScanToken.KuduScanTokenBuilder.includeTabletMetadata(false)

grantatspothero · 2022-02-03T20:05:41Z

Also if we want to split the kudu client upgrade from the background flushing problem let me know.

There just is not a way to [easily] confirm the background flushing problem with kudu client 1.10.0 since the timeouts never/rarely happen.

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/TypeHelper.java

Praveen2112 · 2022-02-08T15:17:36Z

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduPageSink.java

Was this introduced in the latest version ?

No, this config also existed in 1.10.0.

See the PR description, the only way I could reproduce this bug (#5687) was by upgrading the kudu client to 1.15.0 which triggered timeouts to kudu.

That is why I fixed this bug in this upgrade client PR.

Praveen2112 · 2022-02-08T15:19:25Z

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduPageSink.java

Can we extract it from the version update ? It looks like OperationResponse is returned in old APIs too.

Same comment as above, there is no way to easily verify the fix because you need a very specific sequence of timeouts [no timeouts when initially connecting to kudu, but timeouts during deletes/upserts/etc]. The kudu 1.15.0 client [accidentally] provides those timeouts due to a bug in the client.

See here for the failing kudu tests when upgrading to kudu 1.15.0:
https://github.com/trinodb/trino/runs/5057440792?check_suite_focus=true

And here for the workaround that makes tests pass:
https://github.com/trinodb/trino/runs/5060298096?check_suite_focus=true

If we do not mind having no tests around the change, I can extract this into a separate PR.

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduRecordCursor.java

plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestingKuduServer.java

hashhar

I believe all needs to be squashed except the last commit.

LGTM

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/TypeHelper.java

hashhar

LGTM % doc update.

Please update "Requirements" in Kudu docs to specify minimum supported version as 1.15 now. It might work with older versions but we don't test it now - if we intend to claim otherwise then add a test extending BaseConnectorSmokeTest with an older Kudu version.

grantatspothero · 2022-02-11T17:12:33Z

LGTM % doc update.

Please update "Requirements" in Kudu docs to specify minimum supported version as 1.15 now. It might work with older versions but we don't test it now - if we intend to claim otherwise then add a test extending BaseConnectorSmokeTest with an older Kudu version.

Kudu 1.10.0 is pretty old (>2years, released on November 1, 2019) so I just updated the docs to say we only support 1.15.0 or higher

hashhar · 2022-02-11T19:12:04Z

That's fair. 1.13 is oldest supported release anyway according to https://kudu.apache.org/releases.

grantatspothero · 2022-02-11T20:45:37Z

That's fair. 1.13 is oldest supported release anyway according to https://kudu.apache.org/releases.

If we care about supporting 1.13 lmk, I can add an additional set of smoke tests in.

hashhar · 2022-02-12T18:51:36Z

That's fair. 1.13 is oldest supported release anyway according to https://kudu.apache.org/releases.

If we care about supporting 1.13 lmk, I can add an additional set of smoke tests in.

Would be nice to do if it's not too much work otherwise we can tackle that separately. Please remember to update docs accordingly.

hashhar

LGTM % comments.

I believe #10953 depends on this PR getting merged first?

plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKudu115SmokeTests.java

plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKudu113SmokeTests.java

plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestingKuduServer.java

docs/src/main/sphinx/connector/kudu.rst

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduClientConfig.java

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduPageSink.java

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduUpdatablePageSource.java

...ino-kudu/src/main/java/io/trino/plugin/kudu/schema/SchemaEmulationByTableNameConvention.java

wendigo

LGTM % comments

Upgrading the kudu client revealed a few problems: 1. Timeouts to kudu tablets were sometimes occurring during deletes due to a bug in the kudu java client in version 1.13.0. 2. Timeouts were *not* failing query execution because the kudu connector was configured to flush operations in the background. 3. The two combined above meant tests that did deletes sometimes actually did not perform deletes and would fail. This patch upgrades the kudu client, explicitly fails trino execution when kudu rpcs timeout, and marks unsupported data types from kudu 1.15.0.

Does not do anything in kudu 1.15.0

hashhar

LGTM

findepi · 2022-02-24T13:25:49Z

plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/KuduSmokeTests.java

+{
+    private static final String KUDU_VERSION = "1.13.0";
+
+    public static class TestKuduSmokeTestWithDisabledInferSchema


Any reason this is not defined as a top-level class?

See #10940 (comment). To keep all tests for same version together.

Sharing a constant doesn't require nesting classes.
i admire cleverness, but this also means unnecessary class hierarchy, which doesn't help browse the code.

if the paradigm was more frequent in the code base, i would probably get used to it and wouldn't complain.

I noticed depending on how you run the test:
mvn test -Dtest=io.trino.plugin.kudu.KuduLatestConnectorTests
mvn test
some tests will be skipped in the class hierarchy.

(EDIT: BaseConnectorTest has a test to ensure the class name ends in ConnectorTest, for some reason when this is not satisfied for a static inner test class no test failure happens and instead the test gets skipped)

Additionally, when trying to run tests through intellij's UI you can only run the static inner classes (not the top level class).

Seems like the tooling support for static inner test classes is just not good, I'm going to move these to top level classes

cla-bot bot added the cla-signed label Feb 3, 2022

hashhar requested review from Crossoverrr and Praveen2112 February 4, 2022 11:33

grantatspothero force-pushed the gn/updateKuduVersion1.15.0 branch from a67b228 to 7f3eb5e Compare February 4, 2022 18:40

grantatspothero commented Feb 4, 2022

View reviewed changes

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/TypeHelper.java Outdated Show resolved Hide resolved

grantatspothero force-pushed the gn/updateKuduVersion1.15.0 branch from de3d4de to d32db3f Compare February 7, 2022 16:08

Praveen2112 reviewed Feb 8, 2022

View reviewed changes

findepi mentioned this pull request Feb 9, 2022

Remove reflective access from KuduRecordCursor #10986

Merged

grantatspothero force-pushed the gn/updateKuduVersion1.15.0 branch from d32db3f to 9a02003 Compare February 9, 2022 17:18

hashhar reviewed Feb 10, 2022

View reviewed changes

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/TypeHelper.java Outdated Show resolved Hide resolved

This was referenced Feb 10, 2022

Add support for varchar and date types to kudu connector #11009

Closed

Add kerberos auth support to kudu connector #10953

Merged

grantatspothero force-pushed the gn/updateKuduVersion1.15.0 branch 5 times, most recently from 5d09fa3 to 5e2a90c Compare February 10, 2022 19:38

github-actions bot added the docs label Feb 10, 2022

hashhar approved these changes Feb 11, 2022

View reviewed changes

grantatspothero force-pushed the gn/updateKuduVersion1.15.0 branch from 5e2a90c to 6e51247 Compare February 11, 2022 17:11

grantatspothero force-pushed the gn/updateKuduVersion1.15.0 branch from 6e51247 to 07dff60 Compare February 11, 2022 22:30

grantatspothero force-pushed the gn/updateKuduVersion1.15.0 branch from 07dff60 to 5d749ef Compare February 14, 2022 22:27

hashhar approved these changes Feb 15, 2022

View reviewed changes

wendigo reviewed Feb 15, 2022

View reviewed changes

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduPageSink.java Outdated Show resolved Hide resolved

wendigo reviewed Feb 15, 2022

View reviewed changes

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduUpdatablePageSource.java Outdated Show resolved Hide resolved

wendigo reviewed Feb 15, 2022

View reviewed changes

...ino-kudu/src/main/java/io/trino/plugin/kudu/schema/SchemaEmulationByTableNameConvention.java Outdated Show resolved Hide resolved

wendigo approved these changes Feb 15, 2022

View reviewed changes

grantatspothero force-pushed the gn/updateKuduVersion1.15.0 branch 3 times, most recently from c29de25 to 796587a Compare February 15, 2022 19:55

grantatspothero added 2 commits February 15, 2022 17:35

Remove kudu.client.default-socket-read-timeout config

1e28ba5

Does not do anything in kudu 1.15.0

grantatspothero force-pushed the gn/updateKuduVersion1.15.0 branch from 796587a to 1e28ba5 Compare February 15, 2022 23:35

hashhar approved these changes Feb 16, 2022

View reviewed changes

hashhar merged commit 42c0f56 into trinodb:master Feb 16, 2022

github-actions bot added this to the 371 milestone Feb 16, 2022

hashhar mentioned this pull request Feb 16, 2022

Release notes for 371 #10941

Closed

mosabua mentioned this pull request Feb 16, 2022

Add Trino 371 release notes #10943

Merged

This was referenced Feb 22, 2022

Caused by: org.apache.kudu.client.NonRecoverableException: the number of replicas does not equal the number of servers #11137

Closed

Kudu queries fail with expired Scanner under load due to missing keepalives #11136

Closed

findepi reviewed Feb 24, 2022

View reviewed changes

findepi mentioned this pull request Feb 24, 2022

Instantiate collection builder with factory method #11118

Merged

grantatspothero mentioned this pull request Feb 24, 2022

Remove usage of static test classes in kuduconnectortests #11185

Merged

ebyhr mentioned this pull request Mar 2, 2022

Add Kudu unit test class against latest 1.14.0 #7127

Closed

This was referenced Mar 3, 2022

Forbid usage of static inner test classes in TestNG #11316

Merged

Fix flaky kudu tests: schema/table not found and enable CI #11203

Closed

Conversation

grantatspothero commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grantatspothero commented Feb 3, 2022

Uh oh!

grantatspothero commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Praveen2112 Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

grantatspothero Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

Praveen2112 Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

grantatspothero Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

grantatspothero commented Feb 11, 2022

Uh oh!

hashhar commented Feb 11, 2022

Uh oh!

grantatspothero commented Feb 11, 2022

Uh oh!

hashhar commented Feb 12, 2022

Uh oh!

hashhar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wendigo left a comment

Choose a reason for hiding this comment

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

findepi Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

hashhar Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

grantatspothero Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

grantatspothero commented Feb 3, 2022 •

edited

Loading

grantatspothero commented Feb 3, 2022 •

edited

Loading

hashhar left a comment •

edited

Loading

hashhar Feb 24, 2022 •

edited

Loading

grantatspothero Feb 24, 2022 •

edited

Loading