Skip to content
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

Expose Redpanda schema registry #5994

Conversation

gustavomonarin
Copy link
Contributor

@gustavomonarin gustavomonarin commented Oct 17, 2022

This change is intended to make the redpanda implementation of the schema registry easily available on test containers.

The schema registry port 8081 was added to the list of exposed ports and a supporting method getSchemaRegistryAddress was added to easily use for configuring schema.registry.url serializer configuration.

A minor change on the internal advertised address was made as the previous kafka alias was not available within the container and the panda proxy would get lost with the following message:

WARN  2022-10-17 19:08:37,176 [shard 0] kafka/client - broker.cc:52 - std::system_error: kafka: Not found
ERROR 2022-10-17 19:08:37,177 [shard 0] pandaproxy - service.cc:137 - Schema registry failed to initialize internal topic: kafka::client::broker_error ({ node: -1 }, { error_code: broker_not_available [8] })

Since this is only an internal advertised address and the dev-container is an alias to only one node, using 127.0.0.1 should not be an issue. An alternative approach for configuring the kafka alias within the container should work, any suggestion on configuring the internal container address lookup?

Also, a simple test to check if the registry is available is made. Please let me know if you feel like we should actually perform some schema registration.

Copy link
Member

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

thanks for the PR @gustavomonarin ! Exposing the schema registry sounds great. Do you mind improving the docs too, please?

Regarding to the schema registry test. I think it would be great to have such a great example to publish a schema, retrieved and send a message to test that everything is working as expected.

@gustavomonarin
Copy link
Contributor Author

Hi Eddu, thank you for the quick reply :)

Sure. I will try to work on the docs tomorrow.

Regarding the test, i would like to extend the discussion / ask your suggestion:
We could test in different ways:

  1. Test against the schema registry public api (post/get a schema) then assert.
  • This is still only on the api level, over rest.
  1. Actually remove the schema registry test and extend the kafka producing / consumer to use the schema registry.
  • This will require to add the confluent schema registry serializers, a schema (ie protobuf), a generation code plugin for the schema, a proper message. I am afraid of adding the gradle plugin only for test to be honest, beyond some extra energy :).

Do you think is really worth the energy? What are is the value that you are looking for? For developer example/guidance on our tests? For actually checking the redpanda schema registry fully functional?

@gustavomonarin
Copy link
Contributor Author

gustavomonarin commented Oct 17, 2022

By the way, i had a quick look and the pipeline and the errors seem unrelated.

building locally here only the redpanda module works like a charm with ./gradlew :redpanda:check

Do you have any hints regarding the ci?

@eddumelendez
Copy link
Member

the issue is related to connection issues with maven central.

Regarding to the test I think publish and retrieve will be enough then.

@gustavomonarin
Copy link
Contributor Author

Hi @eddumelendez ,

I have updated the documentation and the tests as discussed. Could you please have another look?

gustavo.monarin added 3 commits October 19, 2022 13:10
This change is intended to make the redpanda implementation of the schema registry easily available on test containers.

The schema registry port `8081` was added to the list of exposed ports and a supporting method `getSchemaRegistryAddress` was added to easily use for configuring `schema.registry.url` serializer configuration.

A minor change on the internal advertised address was made as the `kafka` alias was not available within the container and the panda proxy was getting lost with the following message:
```
INFO  2022-10-17 19:08:37,164 [shard 0] kafka/client - broker.cc:41 - connected to broker:-1 - 0.0.0.0:29092
WARN  2022-10-17 19:08:37,176 [shard 0] kafka/client - broker.cc:52 - std::system_error: kafka: Not found
ERROR 2022-10-17 19:08:37,177 [shard 0] pandaproxy - service.cc:137 - Schema registry failed to initialize internal topic: kafka::client::broker_error ({ node: -1 }, { error_code: broker_not_available [8] })
```

Since this is only an internal advertised address and the `dev-container` is an alias to only one node, this should not be an issue. An alternative approach for configuring the `kafka` alias within the container node would be welcome.

Also, a simple test to check if the registry is available is made. Please let me know if you feel like we should actually perform some schema registration.
Describes how to retrieve the schema registry address
Schema registry tests for publishing and consuming new schemas.

The tests only cover the api availability. Further reference should follow the official documentation as this is a quite extensive / complex subject, which would require code generation, plugins to have as example while the [official documentation](https://docs.confluent.io/platform/current/schema-registry/index.html) is quite good.
@gustavomonarin gustavomonarin force-pushed the enable-redpanda-out-of-box-schema-registry branch from 17ce951 to 4453ac1 Compare October 19, 2022 11:10
Copy link
Member

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

Thanks @gustavomonarin ! Just minor suggestions but LGTM in general

@eddumelendez eddumelendez added this to the next milestone Oct 19, 2022
@@ -26,6 +27,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.Assertions.tuple;
import static org.hamcrest.Matchers.hasItems;
Copy link
Member

Choose a reason for hiding this comment

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

Should be replaced with Assertj usage.

Copy link
Member

Choose a reason for hiding this comment

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

@gustavomonarin are you able to fix this? If not, we can do it. The PR is almost done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. Should be fixed on 7a36f32.

Let me know if i missed something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The branch is now slightly behind the origin / master. Should i rebase? (I am not sure if rebase could affect anything with the review process...)

Copy link
Member

Choose a reason for hiding this comment

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

no need to do so. I'm waiting for the checks to pass and I will merge it. Thanks @gustavomonarin !

@eddumelendez eddumelendez merged commit 0bb6925 into testcontainers:main Oct 27, 2022
@eddumelendez
Copy link
Member

thanks @gustavomonarin ! this is now part of main branch

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.

None yet

3 participants