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

[changelog] Let bootstrapping changelog consumer consumes after-image #966

Merged
merged 15 commits into from Apr 30, 2024

Conversation

sixpluszero
Copy link
Contributor

@sixpluszero sixpluszero commented Apr 26, 2024

[changelog] Let bootstrapping changelog consumer consumes after-image

Make bootstrapping consumer also consume after-image data from version topic, instead of before-after image data from cc topic. Fixed a few small bugs during the journey, and clean up the integration tests.
Bugs:

  1. seekToChekpoint needs to be blocked by get(), otherwise consumer will jump to non-deterministic offsets and yield wrong results.
  2. when persisting after-image values to local storage, we should extra bytes from ByteBuffer, instead of converting to array directly.

How was this PR tested?

Added new integration tests and clean up old ones. Now we have 1 integration tests that cover ingestion bootstrapping consumer with/without bootstrap and 1/3 parallel consumers, which replace a few integration tests that serve the same purpose but with a lot of duplication in code.
For the existing TestChangelogConsumer#testAAwithStoreView(), I fixed the flakiness in the test, but the test is still too big and messy, consider refactoring it in the future.

Does this PR introduce any user-facing changes?

Bootstrapping changelog consumer will only consume after-image change events now.

  • No. You can skip the rest of this section.
  • Yes. Make sure to explain your proposed changes and call out the behavior change.

@sixpluszero sixpluszero marked this pull request as draft April 26, 2024 21:56
@gaojieliu
Copy link
Contributor

Is this change still necessary as we are planning to deprecate after-image support?
cc @ZacAttack

@sixpluszero sixpluszero marked this pull request as ready for review April 29, 2024 23:21
@sixpluszero sixpluszero changed the title [WIP][changelog] Let bootstrapping changelog consumer consumes after-image [changelog] Let bootstrapping changelog consumer consumes after-image Apr 29, 2024
@sixpluszero
Copy link
Contributor Author

Is this change still necessary as we are planning to deprecate after-image support? cc @ZacAttack

This is the intended direction to switch from before/after to after-only image for bootstrapping changelog consumer.

Copy link
Contributor

@ZacAttack ZacAttack left a comment

Choose a reason for hiding this comment

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

This overall looks good, just some nits

Copy link
Contributor

@ZacAttack ZacAttack left a comment

Choose a reason for hiding this comment

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

Looks good!

@sixpluszero sixpluszero merged commit eade77a into linkedin:main Apr 30, 2024
32 checks passed
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.

None yet

3 participants