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

initial load for queues #1675

Merged
merged 8 commits into from
May 7, 2024
Merged

initial load for queues #1675

merged 8 commits into from
May 7, 2024

Conversation

serprex
Copy link
Contributor

@serprex serprex commented May 6, 2024

Turns out waitgroup unnecessary since kgo flush handles wait, so removed. Meanwhile, potential send-after-close found in pubsub, so fix

EventHubs is complicated, so skipped. Can be followup

@serprex serprex marked this pull request as draft May 6, 2024 18:51
@serprex serprex force-pushed the queues-initial-load branch 3 times, most recently from c175ea0 to 4b0ccaf Compare May 7, 2024 00:49
@serprex serprex marked this pull request as ready for review May 7, 2024 01:01
@serprex serprex changed the title kafka initial load initial load for queues May 7, 2024
Loop:
for {
select {
case qrecord, ok := <-stream.Records:
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a lot of the code below is shared between pub-sub and Kafka, can we unify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also share quite a bit between cdc/qrep. It'll be a mess of conditions. Would be better as an isolated PR

CommitID: 0,
}

lfn := ls.Env.RawGetString("onRecord")
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't cdc have to do something similar as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but code is mixed up with tracking LSN/counts

@iskakaushik
Copy link
Contributor

makes sense, might be worth following up with a PR on cleaning some of this up :)

@serprex serprex force-pushed the queues-initial-load branch from 6bfc8fe to 3badb8d Compare May 7, 2024 12:39
@serprex serprex enabled auto-merge (squash) May 7, 2024 12:42
@serprex serprex merged commit eae0f32 into main May 7, 2024
8 checks passed
@serprex serprex deleted the queues-initial-load branch May 7, 2024 14:09
iskakaushik pushed a commit that referenced this pull request May 16, 2024
Before we were setting initial load off for queues in handler
With #1675 and #1683, these steps aren't required
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.

2 participants