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

Java: Changed handling of large requests to transfer them as leaked pointers #1696

Closed
wants to merge 52 commits into from

Conversation

jduo
Copy link
Collaborator

@jduo jduo commented Jun 27, 2024

Issue #, if available:
N/A

Description of changes:
Large protobuf messages are extremely slow to decode/encode, and with large messages (e.g., 512MB), the client becomes unresponsive. We resolve this issue by passing the arguments as a leaked pointer instead of a bytes vector when the message size exceeds the limit.

This PR is a port of #1655 for Java

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jduo
Copy link
Collaborator Author

jduo commented Jun 27, 2024

Tests to be added.

@jduo
Copy link
Collaborator Author

jduo commented Jun 27, 2024

@alexey-temnikov
Copy link
Collaborator

@jduo - is it something that could be applicable to a Python client as well?

@jduo
Copy link
Collaborator Author

jduo commented Jun 27, 2024

@jduo - is it something that could be applicable to a Python client as well?

This is a port of the Python work done in #1655 .

@Yury-Fridlyand Yury-Fridlyand added the java issues and fixes related to the java client label Jun 27, 2024
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Please run spotless (java linter) once you finish coding and rust linters (fmt, clippy and doc)

*/
private static void populateRequestWithArgs(List<byte[]> arguments, Command.Builder outputBuilder) {
final long totalArgSize = arguments.stream().mapToLong(arg -> arg.length).sum();
if (totalArgSize < RedisValueResolver.MAX_REQUEST_ARGS_LENGTH) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we count total args amount in a transaction?
Let say MAX_REQUEST_ARGS_LENGTH is 100 and transaction has 20 commands with 20 args each. In that case request will be shipped with 20 arrays of args instead of 20 pointers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does the concept of a transaction exist for Python? The Python PR didn't have this functionality. I'm inclined to treat that as a separate-but-related feature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No idea unfortunately

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Transactions do exist for Python. I created #1713 to track this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW, to be clear, it's not the total number of arguments in a command that is used to determine if a pointer is used. It's the total size in bytes of the arguments (that's why in the test case, we have two arguments to set() but they are 33MB each).

java/src/lib.rs Outdated Show resolved Hide resolved
jonathanl-bq and others added 9 commits June 27, 2024 15:04
* Python: add XREVRANGE command

* Update doc for xrevrange

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Update transaction docs

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

---------

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
* Java: Add `FUNCTION DUMP` and `FUNCTION RESTORE`. (#370)

* Add `FUNCTION DUMP` and `FUNCTION RESTORE` implementations.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Address PR comments.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Add tests.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Address PR comments.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Use GlideString

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Clean up FUNCTION DUMP & RESTORE

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Update handlers

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Update comments

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Add cluster IT test

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* quick review comment

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* SPOTLESS

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
support bitop, bitpos and xdel with GlideString
valkey-io#1630)

support hincrby, hincrbyfloat, incrby and incrbyfloat with GlideString
support llen, strlen, xlen and hstrlen with GlideString
ort-bot and others added 20 commits June 28, 2024 01:35
…lkey-io#1658)

* Python: add XGROUP CREATECONSUMER and XGROUP DELCONSUMER commands

* Rename consumer variable to consumer_name
* Python: Add LOLWUT command (#387)

Added Python LOLWUT command

* Updated CHANGELOG.md

* Fixed formated issues

---------

Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Java: Fix flaky IT. (#390)

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
valkey-io#1642)

* support objectEncoding, objectFreq, objectIdletime and objectRefcount with GlideString

* add to integration tests the use of the API with GlideString parameters

* add binary version integration tests

* nit: spotlessApply

* fix use of GlideString in objectRefcount_returns_null

---------

Co-authored-by: Ubuntu <ubuntu@ip-172-31-152-44.ec2.internal>
…Time, pexpireTime, exists (valkey-io#1637)

* GS version of ttl, pttl, expire, pexpire, expireAt, pexpireAt, expireTime, pexpireTime, exists

Co-authored-by: Ubuntu <ubuntu@ip-172-31-19-27.ec2.internal>
…1635)

* support geodist, getbit, setbit and xack with GlideString

* add to integration tests the use of the API with GlideString parameters

* nit: apply spotlessApply

* remove calls to getBytes()
…lkey-io#1638)

* support move, ltrim, sadd, srem and smove with GlideString

* add to integration tests the use of the API with GlideString parameters

* remove calls to getBytes()

* nit: apply spotlessApply

* add binary and not binary integration test for smove

* add integration tests with GlideString version

* nit: spotlessApply

* remove the use of gs() in ltrim_existing_non_existing_key_and_type_error

* add binary version integration tests
* Java: Add `LCS` command (with IDX option) (#386)

* Implemented LCS with IDX

* TODO: add docs and more integTests

* Added docs and remaining tests

* Addressed comments

* Fixed rust formatting

* Addressed comments

* Added WITHMATCHLEN apis

* Expanded on example

* Fixed rust ci failure

* Removed LcsOptions

* Improved examples in docs

* Examples with different match lengths

* Throw NPE if matches is not present

* Resolved conflicts
* Python: add XREADGROUP command

* Fix mypy error

* PR suggestions
* Java: Add XPENDING command (#389)

* Java: Add XPENDING command

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Add @see doc

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Add @see doc

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Update constant name

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Make tests more robust

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Add UT tests

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Update docs for comments

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

---------

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Update for review coments

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Fix merge conflicts

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Remove file

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Update options to point to valkey

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

---------

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
@jduo jduo marked this pull request as ready for review June 28, 2024 22:24
@jduo jduo requested a review from a team as a code owner June 28, 2024 22:24
@jduo jduo changed the title WIP: Java: Changed handling of large requests to transfer them as leaked pointers Java: Changed handling of large requests to transfer them as leaked pointers Jun 28, 2024
@jduo
Copy link
Collaborator Author

jduo commented Jun 28, 2024

Closing this PR for now to create a PR tracking against bitquill/java/integ_lotjonat_panics

@jduo jduo closed this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java issues and fixes related to the java client
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.