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

[Issue 749]Fix panic caused by flushing current batch with an incorrect internal function. #750

Merged
merged 1 commit into from
May 6, 2022

Conversation

shileiyu
Copy link
Contributor

@shileiyu shileiyu commented Mar 17, 2022

…rrentBatches if the associated batch builder contains multi batches(KeyBasedBatchBuilder).

Doing transparent forwarding could centralized the branching logic at only one place.

<--

Contribution Checklist

  • Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where XYZ should be replaced by the actual issue number.
    Skip Issue XYZ if there is no associated github issue for this pull request.
    Skip component if you are unsure about which is the best component. E.g. [docs] Fix typo in produce method.

  • Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.

  • Each pull request should address only one issue, not mix up code from multiple issues.

  • Each commit in the pull request has a meaningful commit message

  • Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
    -->

Fixes #749

Motivation

Eliminating multiple identical patches, which branch to the correct internal function by checking the type of associated batch builder.

Modifications

Calling internalFlushCurrentBatch will be forwarded to internalFlushCurrentBatches if the associated batch builder contains multiple batches(e.g. KeyBasedBatchBuilder).

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing integration tests.

Does this pull request potentially affect one of the following parts:

None

Documentation

Not required

@shileiyu shileiyu changed the title [Issue 749] [Issue 749]Fix panic due to flushing current batch by using incorrect internal function. Mar 17, 2022
@shileiyu shileiyu changed the title [Issue 749]Fix panic due to flushing current batch by using incorrect internal function. [Issue 749]Fix panic caused by flushing current batch with an incorrect internal function. Mar 21, 2022
…rrentBatches if the associated batch builder contains multi batches(KeyBasedBatchBuilder).

Doing transparent forwarding could centralized the branching logic at only one place.
@shileiyu
Copy link
Contributor Author

Rebased over the top of ci fixing (2.8.2)

@shileiyu
Copy link
Contributor Author

Could someone trigger the CI procedure again?

@wolfstudy wolfstudy added this to the v0.9.0 milestone Apr 13, 2022
@wolfstudy
Copy link
Member

@shileiyu Greate jobs! Can we add unit test to cover this case?

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.

Panic occurred when a producer associated with a KeyBasedBatchBuilder.
4 participants