-
Notifications
You must be signed in to change notification settings - Fork 0
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
Introduced usage of kotlin-kafka #121
Conversation
kfh
commented
Mar 19, 2025
- Swapped out the plain old Kafka client with kotlin-kafka
- Use Hoplite types directly
- Streamlined code (slimmed down and removed redundant code)
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.
Siden emottak-utils
vil bli brukt i nesten alle tjenestene, foreslår jeg å prøve å utføre Kafka klient med så få avhengigheter som mulig, altså ved hjelp av kafka-clients
.
Hvis vi feiler i det så kan vi legge til ekstra biblioteker.
I utgangspunktet er det helt greit, men i dette tilfellet får vi slimmere og OG bedre kode ved å bruke denne tynne wrapperen over kafka-clients. Vi kommer uansett til å bruke dette biblioteket flere steder i kodebasene våre. Målet vårt er jo ikke og ha så få avhengigheter som mulig, men høyest mulig kvalitet på koden. |
Kodekvalitet avgjøres av flere faktorer deriblant gjenbrukbarhet og vedlikeholdbarhet. |
02cc123
to
09f57be
Compare
emottak-utils/src/main/kotlin/no/nav/emottak/utils/kafka/KafkaPublisherClient.kt
Show resolved
Hide resolved
Som sagt, et fint utgangspunkt og ikke ville bloate fellesbiblioteker, men dette bør derimot ikke være en blokker for å benytte oss av biblioteker som gir en betydelig merverdi. I dette tilfeller er det noen åpenbare argumenter slik jeg ser det:
|
emottak-utils/src/main/kotlin/no/nav/emottak/utils/events/EventLoggingService.kt
Outdated
Show resolved
Hide resolved
emottak-utils/src/main/kotlin/no/nav/emottak/utils/kafka/KafkaPublisherClient.kt
Show resolved
Hide resolved
@@ -11,10 +15,12 @@ data class Kafka( | |||
val truststoreType: TruststoreType, | |||
val truststoreLocation: TruststoreLocation, | |||
val truststorePassword: Masked, | |||
val groupId: String | |||
val groupId: String, | |||
val topic: String, |
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.
Synes egentlig at dette feltet burde hete eventTopic
i og med at denne Kafka
-klassen er tenkt gjenbrukt i alle moduler som benytter Kafka, og som da gjerne har andre topics i tillegg til topic for hendelser.
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.
Har vi Kafka-topics som ikke er events? 🤔 Hvis vi følger samme tankegang, burde vi da kanskje også bruke navn som eventGroupId, eventTruststoreType, osv.?
Tenker at det allerede er tydelig at en topic er Kafka-relatert, siden den ligger i dataklassen Kafka.
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.
Dette Kafka
-configobjektet med bootstrapServers
, securityProtocol
osv, og topic
ligger i emottak-utils
fordi dette er felles config som er mer eller mindre statisk (det samme i alle moduler). Det som skiller seg ut, er hvilke topics den enkelte modul skal skrive til.
Det jeg mente, var at vi f.eks. i smtp-transport
har et eget config-objekt KafkaTopics
som inneholder payloadInTopic
, signalInTopic
, payloadOutTopic
og signalOutTopic
. Disse navnene er jo ganske tydelige på hva de er, mens Kafka.topic
er utydelig på hvilken kø den er.
Tanken her er altså at Kafka
sin topic
er den ene felles-topic'en som vi har: Køen for hendelser (events). I allefall slik jeg har oppfattet det. Og da burde feltet også hete eventTopic.
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.
Jeg merger denne nå så vi får koden ut. Jeg har tatt på meg å flytte hele repoet ut av ebxml-processor
til eget repo. Mindre oppgaver som dette kan vi løfte over til nytt repo. Ser for meg litt andre endringer også siden det nå blir et helt selvsetendig repo.
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.
LGTM 👍
Det er alltid fint å redusere boilerplate kode!
53a8e8a
to
8f3798a
Compare