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.

Sign up for GitHub

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

Jump to bottom

Read kafka checked #20

Merged
merged 26 commits into from
Jul 9, 2024
Merged

Read kafka checked #20

merged 26 commits into from
Jul 9, 2024

Conversation

lcoram
Copy link
Collaborator

@lcoram lcoram commented Jun 16, 2024

No description provided.

@lcoram
Copy link
Collaborator Author

lcoram commented Jun 16, 2024

closes: #12

@Lun4m Lun4m linked an issue Jun 19, 2024 that may be closed by this pull request
Checked (kvalobs QC flags) ingestion #12
Closed
@lcoram lcoram marked this pull request as ready for review June 19, 2024 08:13
@lcoram lcoram requested review from intarga and Lun4m June 19, 2024 08:13
Lun4m
Lun4m reviewed Jun 20, 2024
View reviewed changes
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
intarga
intarga requested changes Jun 20, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
db/flags.sql Outdated Show resolved Hide resolved
kafka_checked/Cargo.toml Outdated Show resolved Hide resolved
db/flags.sql Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
intarga
intarga reviewed Jun 25, 2024
View reviewed changes
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
lcoram
lcoram commented Jun 27, 2024
View reviewed changes
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
intarga
intarga reviewed Jun 27, 2024
View reviewed changes
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
lcoram
lcoram commented Jul 3, 2024
View reviewed changes
kafka_checked/src/main.rs Outdated Show resolved Hide resolved
lcoram and others added 18 commits July 4, 2024 14:03
@lcoram @Lun4m
basic parsing of checked data from kafka
4047e18
@lcoram @Lun4m
also parse text data
07944f5
@lcoram @Lun4m
connect to db and try to write kvdata to flags schema
1c4e4f5
@lcoram @Lun4m
code improvements based on clippy
29220cb
@lcoram @Lun4m
code review and better printouts
012e321
@lcoram @Lun4m
try to improve code, avoid unsafe unwraps
88f69cd
@lcoram @Lun4m
use channel to send between kafka and db write threads
84af1a8
@lcoram @Lun4m
simplify code, use tosql for db insert
1bc4b9a
@lcoram @Lun4m
convert 0 back to none for kvalobsid, use manuel's suggestion to dese…
16711a8 
…rialize options
@lcoram @Lun4m
if else for checking kafka consumer errors
64690a3
@Lun4m
Transform level and sensor directly during deserialization
7fc5d8d
@Lun4m
Change wrong datatypes
f78bea1
@Lun4m
Extract parsing logic in separate function
b308412
@Lun4m
Extract read and insert logic in separate function
9fca373
@Lun4m
Fix small inconsistency
5fbc671
@Lun4m
Move package to module
b8e7fde
@Lun4m
match consumer.poll()
79add79
@Lun4m
Add explanation comment
b14754c
Lun4m
Lun4m reviewed Jul 8, 2024
View reviewed changes
ingestion/src/kvkafka.rs Outdated Show resolved Hide resolved
@Lun4m
Copy link
Collaborator

Lun4m commented Jul 8, 2024 •
edited
Loading

Ok, I have rebased on trunk, moved the package to its own module inside the ingestion crate, and fixed some of the types so that they match the ones postgres expects.
I added a test with the simplest possible message we can get, but maybe I should also try testing more complicated structures to make sure everything works correctly.

intarga
intarga reviewed Jul 8, 2024
View reviewed changes
ingestion/src/kvkafka.rs Outdated Show resolved Hide resolved
intarga
intarga approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@intarga intarga left a comment

Choose a reason for hiding this comment

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

Looking good to me, more tests never hurt though if you want to add them. We can merge this when you've added the test cases you want, and you're happy that you've resolved the remaining comments.

@Lun4m Lun4m merged commit 6b91a14 into trunk Jul 9, 2024
1 check passed
@Lun4m Lun4m deleted the read_kafka_checked branch July 9, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lun4m Lun4m Lun4m left review comments

@intarga intarga intarga approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Checked (kvalobs QC flags) ingestion
3 participants
@lcoram @Lun4m @intarga