Skip to content

add avro + kafka + schema registry integration test#10929

Merged
suneet-s merged 9 commits intoapache:masterfrom
clintropolis:avro-schema-registry-integration-test
Mar 8, 2021
Merged

add avro + kafka + schema registry integration test#10929
suneet-s merged 9 commits intoapache:masterfrom
clintropolis:avro-schema-registry-integration-test

Conversation

@clintropolis
Copy link
Copy Markdown
Member

Description

Builds on top of #10314 to add an integration test using a schema-registry docker container to test Apache Kafka + Apache Avro + Confluent Schema Registry secured with basic auth. This container only runs when the kafka-data-format integration test group is run.

I also adjusted the parse method to distinguish failures to get the Avro schema from the registry from actual parse exceptions, since it seems like failure to get the schema would cause no messages to ever be parsable.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@clintropolis clintropolis changed the title add avro + schema registry integration test add avro + kafka + schema registry integration test Mar 4, 2021
@clintropolis clintropolis mentioned this pull request Mar 4, 2021
9 tasks
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, I think some comments in the java code, explaining where the hard coded user names, ports, etc. come from would help. What do you think?

bb.rewind();
// When
GenericRecord record = new SchemaRegistryBasedAvroBytesDecoder(registry).parse(bb);
Assert.assertNull(record);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assert.assertNull(record);
Assert.fail("ParseException should have been thrown");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just delete this line? Similar comment in testParseCorruptedPartial

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted this and other similar lines that would never be reached

public String getSchemaRegistryInternalHost()
{
return "schema-registry:8085";
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add comments here pointing devs to the source of these definitions. I believe it's integration-tests/docker/docker-compose.schema-registry.yml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's docker-compose.base.yml rather than integration-tests/docker/docker-compose.schema-registry.yml that would be relevant here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added javadoc to DockerConfigProvider indicating that the values should be kept in sync with the docker-compose files values.

Comment on lines +62 to +63
"basic.auth.credentials.source", "USER_INFO",
"basic.auth.user.info", "druid:diurd"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to always be in sync with integration-tests/src/test/resources/stream/data/avro_schema_registry/parser/input_row_parser.json?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once avro schema registry supports InputFormat this would be used for that as well, but yeah AvroEventSerializer and AvroSchemaRegistryEventSerializer are a bit more tied to the wikipedia schema used by those tests than some of the other event serializers

# the 'high availability' test cluster with multiple coordinators and overlords
echo "-f ${DOCKERDIR}/docker-compose.high-availability.yml"
elif [ "$DRUID_INTEGRATION_TEST_GROUP" = "kafka-data-format" ]
then
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment above so it doesn't look like this was an accidental empty branch

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just matching the style of the other comment blocks in this script so going to leave like it is to be consistent

@suneet-s suneet-s merged commit 96889cd into apache:master Mar 8, 2021
@clintropolis clintropolis deleted the avro-schema-registry-integration-test branch March 8, 2021 20:00
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants