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

chore(plugin-server): remove INGESTION_DELAY_WRITE_ACKS and workerMethods #17932

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

bretthoerner
Copy link
Contributor

@bretthoerner bretthoerner commented Oct 11, 2023

Problem

INGESTION_DELAY_WRITE_ACKS=true has been rolled out.

Changes

Cleanup.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@bretthoerner bretthoerner requested a review from a team October 11, 2023 19:42
@bretthoerner
Copy link
Contributor Author

bretthoerner commented Oct 11, 2023

I'm worried that the tests depend on the false behavior. I'll look at this later.

@bretthoerner bretthoerner removed the request for review from a team October 11, 2023 20:10
@xvello
Copy link
Contributor

xvello commented Oct 12, 2023

Thanks for cleaning this up!

I think that the failures are related to the DLQ handling, that was committed just after INGESTION_DELAY_WRITE_ACKS:

  • The consumer handles messages just less than 1MB gracefully test name is wrong: we're actually creating a message over 1MB, that fails on the ACK wait
  • To handle that, we need to add a .catch to the promises returned by eachMessage
  • To my knowledge, the only non-retriable kafka error we'd get at this point is "message too big", and sending these to DLQ won't help, as the DLQ topic has the same size as the output topic. We should send an ingestion warning instead
  • While we're here, it's worth auditing this handling code to make sure it does make proper sense, and that the DLQ write errors are properly handled

@bretthoerner bretthoerner force-pushed the brett/INGESTION_DELAY_WRITE_ACKS branch 6 times, most recently from 6b90a86 to a044102 Compare October 12, 2023 22:49
@bretthoerner bretthoerner force-pushed the brett/INGESTION_DELAY_WRITE_ACKS branch from a044102 to 39b3e1e Compare October 13, 2023 02:11
Copy link
Contributor

@xvello xvello 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!

@bretthoerner bretthoerner force-pushed the brett/INGESTION_DELAY_WRITE_ACKS branch from 39b3e1e to bc29323 Compare October 13, 2023 17:31
@bretthoerner
Copy link
Contributor Author

Yup @xvello, tests immediately pass when rebased ontop of #17893

😰 😰 😰

@xvello xvello changed the title chore(plugin-server): remove INGESTION_DELAY_WRITE_ACKS chore(plugin-server): remove INGESTION_DELAY_WRITE_ACKS and workerMethods Oct 16, 2023
@bretthoerner bretthoerner merged commit 286b689 into master Oct 16, 2023
@bretthoerner bretthoerner deleted the brett/INGESTION_DELAY_WRITE_ACKS branch October 16, 2023 15:19
daibhin pushed a commit that referenced this pull request Oct 23, 2023
…hods (#17932)

* chore: stop using piscina worker methods for runEventPipeline

* chore(plugin-server): remove INGESTION_DELAY_WRITE_ACKS

---------

Co-authored-by: Tiina Turban <[email protected]>
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