ockam icon indicating copy to clipboard operation
ockam copied to clipboard

feat(rust): add support for additional kafka addons

Open glenngillen opened this issue 2 years ago • 6 comments

Current behavior

The current support for Apache Kafka is configured via an add-on named confluent.

Proposed changes

I've been testing support for other Apache Kafka compatible brokers and many of them work, but it's a bit weird to configure them by configuring confluent when you're actually using a completely different product. These changes are adding the following subcommands:

  • ockam project addon configure aiven-kafka
  • ockam project addon configure instaclustr
  • ockam project addon configure redpanda
  • ockam project addon configure kafka

glenngillen avatar Sep 07 '23 23:09 glenngillen

There's far more code duplication than I'd like in here (see the configure_(kafka|confluent|aiven|instaclustr|redpanda).rs files and the equivalent files in static). Making generics work with clap was beyond my Rust expertise and sending me down a whole rabbit hole that I suspect more experienced people would be able to quickly refactor or educate me.

glenngillen avatar Sep 07 '23 23:09 glenngillen

Thanks to Davide I've managed to make more of this re-usable across the various kafka-compatible options. Would love some 👀 for feedback, improvements, or approval.

glenngillen avatar Nov 13 '23 04:11 glenngillen

I think we could reduce the code by just having one KafkaCommand and an enum for the vendor type:

Just following up on this specific point: the decision was made to make each add-on it's own subcommand so that peers all get similar treatment (i.e., to avoid having influxdb be its own subcommand while all the kafka ones get relegated to a flag on a generic command)

glenngillen avatar Dec 27 '23 05:12 glenngillen

Ok, I think I've taken this one about as far as I can. Also my ~5% time towards dev stuff isn't enough to keep this in sync with a fast moving develop branch, so the sooner I can get some help to get this up to standard + merged the better.

I've tried to make all the various kafka-generic bits reusable and put on vendor-specific branding over the subcommands. There's a couple of linter warnings I don't actually understand how to resolve though.

glenngillen avatar Dec 27 '23 05:12 glenngillen

@adrianbenavides could you take over landing this please, so we can release it with our next release.

mrinalwadhwa avatar Dec 28 '23 04:12 mrinalwadhwa

One thing I should explicitly call out: in the interests of converging on consisting kafka naming for the generic bits, I also renamed some sql-related bits that have been added since I originally started this PR. I'm not sure if that means we need to consider a migration to rename that table for existing deployments? Or maybe reverting that part of the patch.

glenngillen avatar Dec 28 '23 04:12 glenngillen

I've refactored a little the configure_kafka.rs file to extract the addons subcommnds (confluent, redpanda, ...) into separate files. I've also added a new migration to rename the confluent_config table. It's ready to review now!

adrianbenavides avatar Jan 08 '24 10:01 adrianbenavides