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

SimpleRedisDocumentRepository and SimpleRedisEnhancedRepository saveAll swallow errors #517

Open
davidking-redis opened this issue Oct 15, 2024 · 0 comments

Comments

@davidking-redis
Copy link

When using SimpleRedisDocumentRepository.saveAll or SimpleRedisEnhancedRepository.saveAll, any errors that occur on write requests in the pipeline are swallowed.

We are seeing, when calling SimpleRedisDocumentRepository.saveAll, that the database reaches its maximum memory and not all documents are saved to the database. The redis logs print these warnings:
3344574:S 10 Oct 2024 09:59:40.407 # WARNING: the new maxmemory value set via CONFIG SET (1208585328) is smaller than the current memory usage (2068044275). This will result in key eviction and/or the inability to accept new write commands depending on the maxmemory-policy.
which indicates that the database must be returning OOM errors to write requests. Yet no exception is thrown from saveAll.

We observed the issue with SimpleRedisDocumentRepository, but the code for SimpleRedisEnhancedRepository indicates it will have the same issue.

In both classes, the saveAll method uses a pipeline, but does not check for errors or return the pipeline responses to allow the caller to check for errors.

SimpleRedisDocumentRepository.saveAll calls pipeline.sendCommand which returns a Response. And SimpleRedisEnhancedRepository.saveAll calls pipeline.hmset which also returns a Response. After the pipeline has closed, you can call Response.get to get the response data, and if there was an error then Response.get throws an exception. We could collect up these Responses and iterate over them calling get on each after syncing the pipeline.

Or, instead of using pipeline.sync we could use syncAndReturnAll, which returns a list of all response objects. If any request resulted in an error, the corresponding response object in the list will be a JedisDataException. We could iterate the list and throw any exception found.

Alternatively, saveAll could return a list of Responses or return the list returned by syncAndReturnAll, so the caller can check for errors. But this would mean changing the method signature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant