-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Add binary stream support for XREAD and XREADGROUP (#3566) #4152
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
Conversation
YoHanKi
commented
Apr 27, 2025
- Created StreamEntryBinary class to support binary data with Map<byte[], byte[]>
- Added xreadBinary, xreadBinaryAsMap, xreadGroupBinary, and xreadGroupBinaryAsMap methods to StreamBinaryCommands
- Implemented binary stream builders in BuilderFactory
- Added implementation in Jedis and UnifiedJedis classes
- Created BinaryStreamEntryTest to verify binary stream functionality
- Created StreamEntryBinary class to support binary data with Map<byte[], byte[]> - Added xreadBinary, xreadBinaryAsMap, xreadGroupBinary, and xreadGroupBinaryAsMap methods to StreamBinaryCommands - Implemented binary stream builders in BuilderFactory - Added implementation in Jedis and UnifiedJedis classes - Created BinaryStreamEntryTest to verify binary stream functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a brief look at the PR and it looks good, one think that popped up is to extend the tests and make them follow existing testing pattern
For example use parameterized test to cover both RESP2
/RESP3
and also cover UnifiedJedis
, JedisCluster
and JedisPooled
- For Jedis client streams related test we already have
redis.clients.jedis.commands.jedis.StreamsCommandsTest
- For UnifiedJedis you can take a look as example at
src/test/java/redis/clients/jedis/commands/unified/BinaryValuesCommandsTestBase.java
and relatedClusterBinaryValuesCommandsTest
,PooledBinaryValuesCommandsTest
. Following similar structure for stream commands will test also in cluster&pooled setup
src/test/java/redis/clients/jedis/commands/binary/BinaryStreamEntryTest.java
Outdated
Show resolved
Hide resolved
…er, and Pooled clients TEST (redis#3566) - Introduce `BinaryStreamEntryTest` and `BinaryStreamsCommandsTestBase` following the unified test pattern - Add `ClusterBinaryStreamsCommandsTest` and `PooledBinaryStreamsCommandsTest` to cover JedisCluster and JedisPooled - Parameterize tests to run under both RESP2 and RESP3 protocols - Format BuilderFactory to conform to project formatter rules
src/main/java/redis/clients/jedis/commands/StreamBinaryCommands.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing I forgot: we are planning a mass reformat at some point and will enforce common formatting rules. But for now, please only format the code that has changed, since formatting the entire file makes code reviews harder.
…sequence test (redis#3566) - Refactor StreamsBinaryCommandsTest: replace Map.Entry<byte[], byte[]> with Map.Entry<byte[], StreamEntryID> for xreadBinary, xreadBinaryAsMap, xreadGroupBinary and related methods - Update tests to assert on StreamEntryBinary IDs and byte-array payloads accordingly - Add a new test case covering an illegal UTF-8 sequence (0xc3 0x28) to validate correct binary handling (issue redis#3566)
…#3566) - Introduce a Map<byte[], StreamEntryID> parameter for binary stream commands - Duplicate all existing test cases for xread, xreadAsMap, xreadGroup and xreadGroupAsMap from StreamsCommandsTest.java and replace them with xreadBinary, xreadBinaryAsMap, xreadGroupBinary and xreadGroupBinaryAsMap in StreamsBinaryCommandsTest
…dis#3566) - Duplicate all existing test cases for xread, xreadAsMap, xreadGroup and xreadGroupAsMap from StreamsCommandsTest.java and replace them with xreadBinary, xreadBinaryAsMap, xreadGroupBinary and xreadGroupBinaryAsMap in StreamsBinaryCommandsTest
Hello @ggivo, As you suggested, during the process of writing the duplicated tests, changing the existing public final CommandObject<List<Map.Entry<byte[], List<StreamEntryBinary>>>> xreadBinary(
XReadParams xReadParams,
Map<byte[], StreamEntryID> streams
) (in Apart from replacing calls to Thank you for your valuable guidance and review. |
- Implemented a new JedisByteMap<T> class similar to the existing JedisByteHashMap to properly handle byte array keys in Map<byte[], T> operations - This implementation ensures that `get(byte[])` operations work correctly by using proper byte array equality comparison instead of reference comparison - Fixes issues when using byte arrays as keys in stream-related operations
- Added tests for JedisByteMap similar to existing JedisByteHashMap tests
@YoHanKi Hey, thanks for the update. Will take a look at it later today/tomorrow! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds binary stream support for XREAD
and XREADGROUP
by introducing new response types, commands, builders, and tests.
- Defined
StreamEntryBinary
and added corresponding builders inBuilderFactory
- Extended command interfaces and implementations (
xreadBinary*
andxreadGroupBinary*
) inStreamBinaryCommands
,UnifiedJedis
,Jedis
, andCommandObjects
- Introduced
JedisByteMap
and expanded test coverage withBinaryStreamEntryTest
, pooled/cluster tests, andJedisByteHashMapTest
updates
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
StreamBinaryCommands.java | Added xreadBinary* and xreadGroupBinary* method signatures |
BuilderFactory.java | Added binary stream builders (STREAM_ENTRY_BINARY , etc.) |
CommandObjects.java | Implemented command objects for binary stream commands |
JedisByteMap.java | New Map<byte[],T> implementation |
JedisByteHashMapTest.java | Added tests for JedisByteMap |
Comments suppressed due to low confidence (1)
src/test/java/redis/clients/jedis/collections/JedisByteHashMapTest.java:25
- [nitpick] The test class
JedisByteHashMapTest
now testsJedisByteMap
as well; its name may mislead. Consider renaming to cover both implementations or splitting tests into separate classes.
private static JedisByteMap<byte[]> map2 = new JedisByteMap<>();
src/main/java/redis/clients/jedis/commands/StreamBinaryCommands.java
Outdated
Show resolved
Hide resolved
…ashMap (redis#3566) - Changed Map<byte[], byte[]> in STREAM_ENTRY_BINARY_LIST to JedisByteHashMap.
Hello, @ggivo |
Looks like flaky tests unrelated to your change. I have opened a dedicated issue to investigate the flaky tests & rerun 7.2 job and now it passed. Let me check the other one |
There is already discrepancy in API between `xread` binary variant & `xread` string one. Binary one accepts varargs for requested streams, while the string variant expects them to be provided as `Map`. Going forward xread binary methods should use Map. This commit also marks existing `List<Object> xread` one as deprecated in favor of newly introduced methods for type safety and better stream entry parsing.
@YoHanKi |
Hello @ggivo Thank you for taking the time to review this despite your busy schedule. Is there anything I need to verify or address regarding this issue? |
@YoHanKi Meantime started reviewing your latest change. Unfortunately have some other tasks to complete today, and will continue tomorrow. Thanks for spending time on this! |
* Deprecate List<Object> xreadXXX in favor of newly introduced typed methods
Added tests to cover Pipeline command executions. UnifiedJedis BinaryStreamsCommand test clean up and attempt to simplify focused on testing xreadBinary and xreadGroupBinary methods .
* Merged & refactored a bit Jedis BinaryStreamEntryTest.java & StreamsBinaryCommandsTest to match UnifiedJedis StreamsBinaryCommandsTestBase.
@YoHanKi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hello @ggivo ! |