- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Expose Redpanda schema registry #5994
Conversation
There was a problem hiding this 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.
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:
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? |
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 Do you have any hints regarding the ci? |
the issue is related to connection issues with maven central. Regarding to the test I think publish and retrieve will be enough then. |
Hi @eddumelendez , I have updated the documentation and the tests as discussed. Could you please have another look? |
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.
17ce951
to
4453ac1
Compare
There was a problem hiding this 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
modules/redpanda/src/test/java/org/testcontainers/redpanda/RedpandaContainerTest.java
Outdated
Show resolved
Hide resolved
modules/redpanda/src/test/java/org/testcontainers/redpanda/RedpandaContainerTest.java
Outdated
Show resolved
Hide resolved
modules/redpanda/src/main/java/org/testcontainers/redpanda/RedpandaContainer.java
Show resolved
Hide resolved
@@ -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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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 !
thanks @gustavomonarin ! this is now part of |
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 methodgetSchemaRegistryAddress
was added to easily use for configuringschema.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: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 thekafka
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.