Skip to content
This repository was archived by the owner on Nov 4, 2021. It is now read-only.

Optimize Kafka queue offset handling - #123

Closed
Twixes wants to merge 5 commits into
ingestion-savefrom
optimize-offset-resolving
Closed

Optimize Kafka queue offset handling#123
Twixes wants to merge 5 commits into
ingestion-savefrom
optimize-offset-resolving

Conversation

@Twixes

@Twixes Twixes commented Feb 3, 2021

Copy link
Copy Markdown
Member

Changes

A way to optimize offsets and to avoid weird situations.

Checklist

  • Jest tests

@Twixes
Twixes requested a review from mariusandra February 3, 2021 13:42
@mariusandra mariusandra mentioned this pull request Feb 3, 2021
2 tasks

@mariusandra mariusandra left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I feel like this code is way too complex for what it should do. It also leaves the kafka_offset inside event.

I tried another way to get the same result with #124 ... though I haven't run the code yet.

import { Kafka, Consumer, Message, EachBatchPayload } from 'kafkajs'
import { PluginsServer, Queue, RawEventMessage } from 'types'
import { KAFKA_EVENTS_INGESTION_HANDOFF } from './topics'
import { KAFKA_EVENTS_WAL } from './topics'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why the change from KAFKA_EVENTS_INGESTION_HANDOFF?

@Twixes

Twixes commented Feb 3, 2021

Copy link
Copy Markdown
Member Author

Oh, sry, actually I was doing some debugging due to tests failing, so this is not in a reviewable state ATM (was reviewable at 6feda22).

If this is too complex, what would your alternative suggestion be? Oh, yeah, noticed #124.
This isn't super simple, but would cover all cases pretty well (and there are various ones we have to consider, e.g plugin processing may: change no events, or add events that there never were in the Kafka queue, or remove a series of events that were in the Kafka queue, or remove every other event, or remove all of them).

@mariusandra

Copy link
Copy Markdown
Collaborator

Which cases would #124 not cover?

@mariusandra

Copy link
Copy Markdown
Collaborator

just saw your reply :), will give it a thought after dinner

const singleIngestionTimer = new Date()
await this.saveEvent(event)
resolveOffset(event.kafka_offset!)
resolveOffset(eventUuidToKafkaOffset.get(event.uuid!)!)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TODO: don't run this if the event was not present in batch from Kafka (meaning it was inserted by a plugin's processEventBatch)

@Twixes Twixes closed this Feb 3, 2021
@Twixes
Twixes deleted the optimize-offset-resolving branch February 19, 2021 03:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants