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

[Client] Fix send chunking message failed when ordering key is set. #13699

Merged

Conversation

Jason918
Copy link
Contributor

Master Issue: #13688

Motivation

The root cause is the same as #13688. Chunking messages share the same metadata. And the type of ordering key is bytebuf, which can not be serialized twice.

Modifications

Reset the field before sending.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • See testChunkingWithOrderingKey

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc
    bug fix

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 11, 2022
@Jason918
Copy link
Contributor Author

@gaoran10 @MarvinCai @codelipenghui This is similar to the fix with schemaVersion #12720. PTAL

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui added this to the 2.10.0 milestone Jan 12, 2022
@codelipenghui codelipenghui added release/2.8.3 type/bug The PR fixed a bug or issue reported a bug labels Jan 12, 2022
@codelipenghui
Copy link
Contributor

@RobertIndie Please also help review this PR.

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Jan 14, 2022

From my perspective, it's so terrible to have all these issues just because all chunks share the same MessageMetadata. I'd like just deep copying this object instead of sharing the object and setting some fields carefully.

I also realized this potential issue when I implement the chunking message feature for C++ client, so I do the deep copy for the initial implementation.

Though for the sake of overhead of bytes copy, this solution looks good. But does it really affect? So I won't approve but I won't request changes as well.

@Jason918
Copy link
Contributor Author

I'd like just deep copying this object instead of sharing the object and setting some fields carefully.

Yes, it's better to do it this way in the first implementation of chunking.
But before this fix, I have checked all the other bytes field. This is the last one.
And since there is no direct way to deep copy of this protobuffer object.
So I think it's OK to do it by resetting field for now.

Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

Lgtm

@BewareMyPower BewareMyPower merged commit 8dc2975 into apache:master Jan 18, 2022
codelipenghui pushed a commit that referenced this pull request Jan 18, 2022
Master Issue: #13688

### Motivation

The root cause is the same as #13688. Chunking messages share the same metadata. And the type of ordering key is bytebuf, which can not be serialized twice.

### Modifications

Reset the field before sending.

(cherry picked from commit 8dc2975)
@codelipenghui codelipenghui added cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life labels Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.3 release/2.9.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants