Skip to content

force native order when wrapping ByteBuffer#8055

Merged
clintropolis merged 1 commit intoapache:masterfrom
AlexanderSaydakov:fix_bytebuffer_order
Jul 12, 2019
Merged

force native order when wrapping ByteBuffer#8055
clintropolis merged 1 commit intoapache:masterfrom
AlexanderSaydakov:fix_bytebuffer_order

Conversation

@AlexanderSaydakov
Copy link
Copy Markdown
Contributor

Fixes #8032

force native order when wrapping ByteBuffer since Druid might have it set incorrectly

@AlexanderSaydakov
Copy link
Copy Markdown
Contributor Author

This was already forced every time a ByteBuffer from Druid is wrapped for use with Datasketches library, except this one instance that was missed by mistake.

@jihoonson
Copy link
Copy Markdown
Contributor

Would you please add a unit test?

@jihoonson
Copy link
Copy Markdown
Contributor

There is a helper method called AggregatiohnTestHelper.runRelocateVerificationTest() which could facilitate writing unit tests for this.

@himanshug
Copy link
Copy Markdown
Contributor

himanshug commented Jul 10, 2019

this is unfortunate (discussed in #6381 (comment) as well ) . I wish there could be a better solution to handle this.

@AlexanderSaydakov
Copy link
Copy Markdown
Contributor Author

Would you please add a unit test?

Unfortunately I don't have a unit test. The bug leads to sporadic failures. I could not find a small use case to reproduce it.

@himanshug
Copy link
Copy Markdown
Contributor

@AlexanderSaydakov were you able to verify that it does fix #8032 , sounds like it was not reproducible ?

@AlexanderSaydakov
Copy link
Copy Markdown
Contributor Author

I managed to get a similar failure in a micro-quickstart environment using the same 20 sketches from a test: https://github.com/apache/incubator-druid/blob/master/extensions-core/datasketches/src/test/resources/quantiles/doubles_sketch_data.tsv
The proposed change fixes that case. But I could not find a scenario that would fail in a test in development environment. I am almost sure that the proposed change should fix #8032

@quenlang would it be possible for you to give this a try and let us know if this change fixes your problem? Thank you.

@himanshug
Copy link
Copy Markdown
Contributor

I see, but anyways, this fixes some bug if not #8032 .

Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

👍

@clintropolis clintropolis merged commit 4b176ad into apache:master Jul 12, 2019
@quenlang
Copy link
Copy Markdown

@AlexanderSaydakov I had a try with this patch, the query worked. Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Possible corruption: Memory capacity too small: 47136 < 12845088" exception occur in 0.14.x but not in 0.13.x

5 participants