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

DRAFT KAFKA-18692: Consider to unify KStreamImpl "repartitionRequired" with GraphNode "keyChangingOperation" #18800

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

appchemist
Copy link
Contributor

That PR is a POC.

For now, I only focused on modifying KStreamImpl.repartitionRequired to replace it with GraphNode.keyChangingOperation.

So I'll be working on it further to make sure it's the right fix and need to write some test code.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@github-actions github-actions bot added triage PRs from the community streams labels Feb 4, 2025
@appchemist
Copy link
Contributor Author

Hi, @mjsax
If you have a moment, Please take a look

@mjsax
Copy link
Member

mjsax commented Feb 5, 2025

Thanks for this draft -- might take a few days until I find time to take a look.

@github-actions github-actions bot removed the triage PRs from the community label Feb 6, 2025
… GraphNode "keyChangingOperation"

- refactor to re-use getKeyChangingParentNode
- refactor to GraphNode
@appchemist
Copy link
Contributor Author

appchemist commented Feb 17, 2025

Hi, @mjsax
After reuse InternalStreamsBuilder#getKeyChangingParentNode(). Some Tests are failed.
So, I'll looking for these failed tests
I haven't had much time lately, so I haven't looked at it much.

After Analyzing the cause, I'll request the review again

However, I would appreciate it if you could take a look at the following comment when you have time.
#18800 (comment)

… GraphNode "keyChangingOperation"

- refactor
… GraphNode "keyChangingOperation"

- refactor GraphNode
@github-actions github-actions bot added the small Small PRs label Feb 22, 2025
@appchemist
Copy link
Contributor Author

appchemist commented Feb 22, 2025

:streams:testAll now succeed.

@ mjsax
If you have a moment, Please take a look.

If the POC is worth applying, I'll resolve the conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants