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

Fixed: #8535 - Increased buffer size from 500 to 5000 #8536

Closed
wants to merge 1 commit into from
Closed

Fixed: #8535 - Increased buffer size from 500 to 5000 #8536

wants to merge 1 commit into from

Conversation

wolframite
Copy link

Added unit test to show that the buffer in the TextReadHandler is too small to bulk-get 100 keys and Increased buffer size to 5000

Fix for Issue #8535

…dHandler is too small to bulk-get 100 keys / Increased buffer size to 5000
@ahmetmircik ahmetmircik added this to the 3.7 milestone Jul 28, 2016
@@ -62,7 +62,7 @@

private static final Map<String, CommandParser> MAP_COMMAND_PARSERS = new HashMap<String, CommandParser>();

private static final int CAPACITY = 500;
private static final int CAPACITY = 5000;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about having a static capacity. a dynamically growing buffer might be better, I think.

Copy link
Author

Choose a reason for hiding this comment

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

I thought the ByteBuffer was used for performance reasons. If you say dynamic, would you use a StringBuilder with a start capacity?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I means is, since TextReadHandler is single threaded, ByteBuffer can be replaced with a bigger one when needed (for example, create with double capacity, copy contents and destroy the old one). Still, a max-threshold will be needed to avoid creating very large buffers.

Or the whole logic can be changed to use a relatively small (1k, 4k whatever) buffer to build command incrementally. I am not sure if it's really needed to hold all data read from socket in a single large buffer.

@sancar
Copy link
Contributor

sancar commented Dec 2, 2016

Hi thanks for the report and fix. A chosen hardcoded value already proven not to work. The one we choose was not suitable for your usecase. The new one will be a problem for someone else in the future. Making it working with any data should be the way forward as @mdogan suggested. I am closing this one and linking the new issue here #9355

@sancar sancar closed this Dec 2, 2016
@mmedenjak mmedenjak added the Source: Community PR or issue was opened by a community user label Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Source: Community PR or issue was opened by a community user Team: Client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants