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.

Sign up for GitHub

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

Jump to bottom

Issue 8974: Peeking at compressed messages throws an exception (Readonly buffers not supported by Airlift) #8990

Merged
merged 1 commit into from
Dec 22, 2020
Merged

Issue 8974: Peeking at compressed messages throws an exception (Readonly buffers not supported by Airlift) #8990

merged 1 commit into from
Dec 22, 2020

Conversation

eolivelli
Copy link
Contributor

Fixes #8974

Motivation

In certain cases peeking messages on compresses topics return an error, see #8974 because Airlift does not support readonly ByteBuffers, because they do not give access to the underlying array)

Modifications

Copy the ByteByffer in case of unsupported buffer type

Verifying this change

This change adds new tests that reproduce the error and demonstrate that the problem is fixed.

@eolivelli
Copy link
Contributor Author

@merlimat @codelipenghui @rdhabalia PTAL
I had to copy the ByteBuffer, no other way :-(

@merlimat
Copy link
Contributor

I had to copy the ByteBuffer, no other way :-(

That is ok, this is only used when peeking messages. For regular producer/consumer compression it's done on the direct memory instead, without copy.

merlimat
merlimat approved these changes Dec 17, 2020
View reviewed changes
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

We could also add a test to peek compressed messages, so we have the end-to-end validation.

@merlimat merlimat added the type/bug The PR fixed a bug or issue reported a bug label Dec 17, 2020
@merlimat merlimat added this to the 2.8.0 milestone Dec 17, 2020
@eolivelli
Copy link
Contributor Author

@merlimat I wasn't able to reproduce the problem with and end-to-end test (using the existing end-to-end tests about compression).
If you feel strong I can try to reproduce it better, but probably I will have to start a full (with real BK/ZK...) I am not sure it is worth

@merlimat
Copy link
Contributor

but probably I will have to start a full (with real BK/ZK...) I am not sure it is worth

There are already tests that are spinning up real BK, just need to inherit from BkEnsemblesTestBase

@wolfstudy
Copy link
Member

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

3 similar comments
@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie sijie merged commit cbc606b into apache:master Dec 22, 2020
@eolivelli eolivelli deleted the fix/issue-8974-readonly-bytebuf-decompress branch December 22, 2020 20:18
codelipenghui pushed a commit that referenced this pull request Dec 23, 2020
@eolivelli @codelipenghui
Issue 8974: Peeking at compressed messages throws an exception (Reado…
fbbc251 
…nly buffers not supported by Airlift) (#8990)

Fixes #8974 

### Motivation
In certain cases peeking messages on compresses topics return an error, see #8974 because Airlift does not support readonly ByteBuffers, because they do not give access to the underlying array)

### Modifications

Copy the ByteByffer in case of unsupported buffer type

### Verifying this change

This change adds new tests that reproduce the error and demonstrate that the problem is fixed.

(cherry picked from commit cbc606b)
codelipenghui pushed a commit that referenced this pull request Dec 23, 2020
@eolivelli @codelipenghui
Issue 8974: Peeking at compressed messages throws an exception (Reado…
057b33e 
…nly buffers not supported by Airlift) (#8990)

Fixes #8974 

### Motivation
In certain cases peeking messages on compresses topics return an error, see #8974 because Airlift does not support readonly ByteBuffers, because they do not give access to the underlying array)

### Modifications

Copy the ByteByffer in case of unsupported buffer type

### Verifying this change

This change adds new tests that reproduce the error and demonstrate that the problem is fixed.

(cherry picked from commit cbc606b)
@codelipenghui codelipenghui mentioned this pull request Feb 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat approved these changes

@sijie sijie sijie approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

Assignees

@eolivelli eolivelli

Labels
cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.6.3 release/2.7.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

Peeking at compressed messages throws an exception (Readonly buffers not supported by Airlift)
5 participants
@eolivelli @merlimat @wolfstudy @codelipenghui @sijie