Improve error message in Kafka connector when parsing recursive protobuf schemas#15724
Conversation
|
This is resubmission of #15312 with conflicts and review comments addressed. |
hashhar
force-pushed
the
hashhar/kafka-avoid-schema-parsing-loop
branch
3 times, most recently
from
a38b4ec to
2e72ce1
Compare
.../io/trino/plugin/kafka/protobuf/TestKafkaProtobufWithSchemaRegistryMinimalFunctionality.java
Outdated
Show resolved
Hide resolved
...n/trino-kafka/src/main/java/io/trino/plugin/kafka/encoder/protobuf/ProtobufSchemaParser.java
Outdated
Show resolved
Hide resolved
Praveen2112
left a comment
Praveen2112
left a comment
There was a problem hiding this comment.
Looks good to me after applying @nevillelyh 's comment.
plugin/trino-kafka/pom.xml
Outdated
There was a problem hiding this comment.
nit: Can we add a issue to remove this workaround on generating the code ? The way we invoke parser should support it out of the box, I have a rough fix for it and will be working on a PR for the same.
There was a problem hiding this comment.
Sure, I'll create an issue + add TODO.
cc: @adamjshook regarding some schema parser improvements Praveen plans to make which might be relevant for you.
There was a problem hiding this comment.
Yup, that'd be relevant as I am using the same plugin. I'll keep an eye out for it.
hashhar
force-pushed
the
hashhar/kafka-avoid-schema-parsing-loop
branch
from
2e72ce1 to
fb3b330
Compare
|
@nevillelyh / @Praveen2112 PTAL at the fixup which improves error message and also stops relying on ImmutableSet's insertion order iteration. |
There was a problem hiding this comment.
This error message goes to user, they are not much aware of trino java packages. Should use field names from protobuf schema?
There was a problem hiding this comment.
The field names are from the protobuf schema, see https://github.com/trinodb/trino/pull/15724/files#diff-8ca538d4195acbb011666dca6daee504059c8090ffbedda1cf1fb8389c1e4827R3.
Kafka protobuf schema cannot be translated to a Trino type when the protobuf schema uses recursive reference to some types. Co-authored-by: Maxim Lukyanenko <mlukyanenko@gmail.com> Co-authored-by: Ashhar Hasan <ashhar.hasan@starburstdata.com>
Writing protobuf messages using TestingKafka throws NullPointerException if the schema uses an `import` directive. Co-authored-by: Maxim Lukyanenko <mlukyanenko@gmail.com> Co-authored-by: Ashhar Hasan <ashhar.hasan@starburstdata.com>
Co-authored-by: Maxim Lukyanenko <mlukyanenko@gmail.com> Co-authored-by: Ashhar Hasan <ashhar.hasan@starburstdata.com>
hashhar
force-pushed
the
hashhar/kafka-avoid-schema-parsing-loop
branch
from
fb3b330 to
35a8d05
Compare
|
auto-squashed, will merge once CI is done. |
Kafka connector throws a StackOverflowError when the protobuf schema includes self referenced object types. To improve user experience the schema parsing now throws TrinoException saying that the type is unsupported. Co-authored-by: Maxim Lukyanenko <mlukyanenko@gmail.com> Co-authored-by: Ashhar Hasan <ashhar.hasan@starburstdata.com>
hashhar
force-pushed
the
hashhar/kafka-avoid-schema-parsing-loop
branch
from
35a8d05 to
24d3033
Compare
hashhar
changed the title
| <!-- TODO: Instead of using this plugin to generate classes we should invoke the parser directly (https://github.com/trinodb/trino/issues/16039) --> | ||
| <plugin> | ||
| <groupId>com.github.os72</groupId> | ||
| <artifactId>protoc-jar-maven-plugin</artifactId> |
There was a problem hiding this comment.
Let's use a specific version of this plugin - #16043
7 participants
Description
Kafka connector throws a StackOverflowError when the protobuf schema
includes self referenced object types.
To improve user experience the schema parsing now throws TrinoException
saying that the type is unsupported.
Release notes
(x) Release notes are required, with the following suggested text: