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

IS-1646: Consume huskelapp-status in personoversiktstatus #316

Merged
merged 4 commits into from
Oct 9, 2023

Conversation

eirikdahlen
Copy link
Contributor

@eirikdahlen eirikdahlen commented Oct 6, 2023

Hva har blitt lagt til✨🌈

Har satt opp en kafkaconsumer for topicet som ishuskelapp konsumerer på. Her kommer det inn huskelapper med et flagg isActive for en person, som skal bestemme om oppgaven statusen skal vises i syfooversikt.

Funker i dev ✅

@eirikdahlen eirikdahlen requested a review from a team as a code owner October 6, 2023 07:59
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hvordan kan man evt implementere Repository-konseptet i slike consumers der vi ønsker å ha én connection oppe hele tiden? Jeg føler jo at logikken som skjer inne i forEachen hører hjemme i en service 🤷🏼‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Måtte kanskje hatt en repository-metode ala createOrUpdatePersonoppgaver(huskelapp: List<Huskelapp>), men lit enig i at den logikken hører mer hjemme her...

Comment on lines 29 to 33
if (tombstones.isNotEmpty()) {
val numberOfTombstones = tombstones.size
log.error("Value of $numberOfTombstones ConsumerRecord are null, most probably due to a tombstone. Contact the owner of the topic if an error is suspected")
COUNT_KAFKA_CONSUMER_HUSKELAPP_TOMBSTONE.increment(numberOfTombstones.toDouble())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vi sender vel ikke tombstones på denne topicen, så vet ikke om vi trenger dette?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nei, jeg tenker vi kan fjerne det.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Endret nå, prøvde å ta i bruk en requireNoNulls, den kaster feil hvis det plutselig skulle komme null-verdier her.
Returns an original collection containing all the non-null elements, throwing an IllegalArgumentException if there are any null elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

)
val consumerProperties = Properties().apply {
putAll(kafkaAivenConsumerConfig(kafkaEnvironment = kafkaEnvironment))
this[ConsumerConfig.MAX_POLL_RECORDS_CONFIG] = "1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Én av gangen, eller skulle vi tatt flere? 🤷🏼‍♂️

Copy link
Contributor

@andersrognstad andersrognstad Oct 6, 2023

Choose a reason for hiding this comment

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

Ser vi går for 1 noen steder og 10 andre steder... husker ikke om det var noen spesielle grunner til at man burde begrense til 1 🤔 Kanskje noe samtidighets-problematikk. Går jo an å prøve 10 (eller enda flere) så skrur vi heller ned hvis det blir et problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey 👍🏼

Copy link
Contributor

Choose a reason for hiding this comment

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

Ja, i dette tilfellet går det sikkert bra med 10 eller flere. Vi bør begrense det til 1 dersom konsumeringen gjør noe mer enn å skrive til databasen, feks lager en ny record på et annet topic eller skriver til MQ eller noe annet "eksternt".

import java.util.UUID

data class KafkaHuskelapp(
val uuid: UUID,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Så at det var UUID i ishuskelapp. Deserialisereren fikser det, eller?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tror det skal gå bra, men vi får teste i dev 💥

Comment on lines +12 to +16
fun toPersonoversiktStatus() = PersonOversiktStatus(
fnr = personIdent.value,
).copy(
huskelappActive = isActive,
)
Copy link
Contributor Author

@eirikdahlen eirikdahlen Oct 6, 2023

Choose a reason for hiding this comment

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

Gjorde sånn i stedet for å "fylle ut" det store objektet, nå som vi har default-values.
Hadde det vært penere med en .create()-metode? I så fall burde man kanskje lage flere, for aktivitetskrav, huskelapp, dialogmøtestatusendring etc etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Hadde nok vært fint og ryddet litt opp i det ja, men får ta det i en egen PR.

Copy link
Contributor

@andersrognstad andersrognstad left a comment

Choose a reason for hiding this comment

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

Ser bra ut!

@eirikdahlen eirikdahlen merged commit 1700de6 into master Oct 9, 2023
4 checks passed
@eirikdahlen eirikdahlen deleted the IS-1646-huskelapp-consumer branch October 9, 2023 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants