feat(operator): plain kafkasql support #5375

jsenko · 2024-10-21T16:41:03Z

No description provided.

andreaTP · 2024-10-22T10:45:09Z

.../model/src/main/java/io/apicurio/registry/operator/api/v1/ApicurioRegistry3SpecKafkaSql.java

+import lombok.*;
+
+@JsonInclude(Include.NON_NULL)
+@JsonPropertyOrder({ "env", "host" })


I think this is a leftover

andreaTP · 2024-10-22T10:47:55Z

operator/model/src/main/java/io/apicurio/registry/operator/api/v1/ApicurioRegistry3SpecApp.java

+    @JsonPropertyDescription("""
+            Configure KafkaSQL storage.""")
+    @JsonSetter(nulls = Nulls.SKIP)
+    private ApicurioRegistry3SpecKafkaSql kafkasql = new ApicurioRegistry3SpecKafkaSql();


In this case, being able to identify the presence of the field checking for null at the kafkasql object level seems useful to me.

Fixed. So the rule is: default value for collections, null for objects? If so we need to update ApicurioRegistry3Spec.

andreaTP · 2024-10-22T10:55:55Z

operator/controller/src/test/java/io/apicurio/registry/operator/it/KafkaSQLITTest.java

+    }
+
+    private static void applyStrimziResources(boolean delete) {
+        String text = load("/k8s/examples/kafkasql/strimzi-cluster-operator-0.43.0.yaml");


The strimzi-cluster-operator is huge, I think that, in this specific case, we can use the upstream installation mechanism: https://strimzi.io/install/latest.
We can keep an eye on it and, if it starts failing too often, we can revert to dumping resources.

Done. However, I would prefer downloading a specific version, since we're using Strimzi Java model.

we're using Strimzi Java model

that's something we should aim to avoid. No need from our perspective to bind to Strimzi.

andreaTP · 2024-10-22T10:59:49Z

operator/controller/src/test/java/io/apicurio/registry/operator/it/KafkaSQLITTest.java

+
+    @AfterAll
+    public static void afterAll() throws Exception {
+        applyStrimziResources(true);


Two comments here:

do we need to explicitly delete resources? something is getting stuck on finalizers?

deletion of resources in a namespace should remove detected resources, e.g. we should get from the namespace and remove. The source of truth is the cluster and not the previously applied yaml.

andreaTP · 2024-10-22T11:04:29Z

operator/controller/src/test/java/io/apicurio/registry/operator/it/ITBase.java

@@ -54,7 +54,7 @@ public enum OperatorDeployment {
    private static Operator operator;

    @BeforeAll
-    public static void before() throws Exception {
+    public static void beforeAll() throws Exception {


nitpick: the operator codebase is still early and I don't mind those changes, going forward let's try to minimize those unrelated changes, and, maybe pack them in a "chore" separate PR.
Two reasons:

git history -> is easier to follow the history of changes when they are done in isolation

review -> it keeps things better scoped

Reverted. I consider this fine, since it is often easier to fix a small thing immediately if you notice it, rather than trying to remember and create micro PRs. Do you consider it a good practice to track things like these in issues? If so I'll create an issue for this.

no need to process for this, just open a PR changing just this bit and we fast-track it.

andreaTP · 2024-10-22T11:05:53Z

operator/controller/src/main/resources/k8s/examples/kafkasql/plain/ephemeral.kafka.yaml

@@ -0,0 +1,36 @@
+apiVersion: kafka.strimzi.io/v1beta2


Should those examples resources stay in the test scope?
Do we need them at runtime in the Operator?

Not necessarily in runtime, but my thinking is that we can use those examples in documentation for example, while making sure they are tested. If you have a suggestion on how to better organize this, let me know.

Consider the test/resources folder, so that they are not going to be bundled with the final JARs

operator/controller/src/main/java/io/apicurio/registry/operator/feat/KafkaSql.java

andreaTP · 2024-10-22T11:08:54Z

operator/controller/src/test/java/io/apicurio/registry/operator/it/KafkaSQLITTest.java

+        client.resource(kafka).create();
+
+        await().ignoreExceptions().until(() -> {
+            var status = client.resources(Kafka.class).inNamespace(namespace).withName("my-cluster").get()


Can we have a check that is not dependent on the Strimzi CR?
E.g. something that should work without code changes also for plain Kafka and RedPanda.

I would welcome a specific suggestion/code from you. I added the dependency because:

I need to read the status in the test. The alternative is using raw JsonNodes, or assuming what the bootstrapServers value will be, and I don't like either of those much.

Our customers are very likely to use Strimzi/AMQ streams, so using it in our tests gives greater confidence it'll work, and provides examples for users.

We can get the status of the ReplicaSet of the Kafka pods, all the checks are, anyhow happening with retries and it should be more than enough to have a stable situation.

Strimzi does not use ReplicaSet btw.

andreaTP · 2024-10-22T11:09:32Z

operator/controller/pom.xml

@@ -89,6 +89,12 @@
      <artifactId>lombok</artifactId>
      <scope>provided</scope>
    </dependency>
+    <dependency>
+      <groupId>io.strimzi</groupId>


Ideally we should be able to not tight the setup to Strimzi but keep it more generic.

andreaTP

This PR looks in great shape now, thanks!

The last bit is to remove the dependency on Strimzi, I can help doing it if required, just let me know 👍

jsenko · 2024-10-24T11:39:51Z

This PR looks in great shape now, thanks!

The last bit is to remove the dependency on Strimzi, I can help doing it if required, just let me know 👍

Done.

andreaTP

Good to go, nice job!

andreaTP · 2024-10-24T12:20:19Z

operator/controller/src/test/java/io/apicurio/registry/operator/it/KafkaSQLITTest.java

+        final var clusterNAme = "my-cluster";
+
+        await().ignoreExceptions().untilAsserted(() ->
+        // Strimzi uses StrimziPodSet instead of ReplicaSet, so we have to check pods


…rces

jsenko requested a review from andreaTP October 21, 2024 16:41

apicurio-bot bot added the area/storage label Oct 21, 2024

andreaTP reviewed Oct 22, 2024

View reviewed changes

andreaTP requested changes Oct 24, 2024

View reviewed changes

andreaTP approved these changes Oct 24, 2024

View reviewed changes

jsenko added 4 commits October 24, 2024 15:26

feat(operator): plain kafkasql support

482d461

refactor(operator): code improvements

ce733dd

refactor(operator): remove dependency on Strimzi Java model

9185034

chore(operator): move example files from main/resources to test/resou…

1c46f7c

…rces

jsenko force-pushed the operator-kafkasql-plain branch from cbc4ab5 to 1c46f7c Compare October 24, 2024 13:26

jsenko enabled auto-merge (rebase) October 24, 2024 13:26

jsenko merged commit ef34ecd into Apicurio:main Oct 24, 2024
19 checks passed

jsenko deleted the operator-kafkasql-plain branch October 24, 2024 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(operator): plain kafkasql support #5375

feat(operator): plain kafkasql support #5375

jsenko commented Oct 21, 2024

andreaTP Oct 22, 2024

jsenko Oct 23, 2024

andreaTP Oct 22, 2024

jsenko Oct 23, 2024

andreaTP Oct 22, 2024

jsenko Oct 23, 2024

andreaTP Oct 24, 2024

andreaTP Oct 22, 2024

jsenko Oct 23, 2024 •

edited

Loading

andreaTP Oct 22, 2024

jsenko Oct 23, 2024

andreaTP Oct 24, 2024

andreaTP Oct 22, 2024

jsenko Oct 23, 2024

andreaTP Oct 24, 2024

andreaTP Oct 22, 2024

jsenko Oct 23, 2024

andreaTP Oct 24, 2024

jsenko Oct 24, 2024

andreaTP Oct 22, 2024

andreaTP left a comment

jsenko commented Oct 24, 2024

andreaTP left a comment

andreaTP Oct 24, 2024

feat(operator): plain kafkasql support #5375

feat(operator): plain kafkasql support #5375

Conversation

jsenko commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsenko Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaTP left a comment

Choose a reason for hiding this comment

jsenko commented Oct 24, 2024

andreaTP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsenko Oct 23, 2024 •

edited

Loading