ZUNION/ZINTER commands introduced #1469

sokomishalov · 2020-10-23T01:36:23Z

codecov · 2020-10-23T02:13:30Z

Codecov Report

Merging #1469 into main will decrease coverage by 0.00%.
The diff coverage is 74.77%.

@@             Coverage Diff              @@
##               main    #1469      +/-   ##
============================================
- Coverage     78.77%   78.77%   -0.01%     
- Complexity     6225     6242      +17     
============================================
  Files           462      463       +1     
  Lines         20790    20847      +57     
  Branches       2300     2300              
============================================
+ Hits          16378    16422      +44     
- Misses         3347     3362      +15     
+ Partials       1065     1063       -2

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/io/lettuce/core/ZStoreArgs.java	`0.00% <0.00%> (-90.25%)`	`0.00 <0.00> (-15.00)`
...coroutines/RedisSortedSetCoroutinesCommandsImpl.kt	`3.38% <0.00%> (-0.54%)`	`2.00 <0.00> (ø)`
...va/io/lettuce/core/AbstractRedisAsyncCommands.java	`95.97% <60.00%> (-0.80%)`	`429.00 <6.00> (+4.00)`	⬇️
...io/lettuce/core/AbstractRedisReactiveCommands.java	`88.84% <60.00%> (-0.65%)`	`419.00 <6.00> (+4.00)`	⬇️
src/main/java/io/lettuce/core/ZAggregateArgs.java	`86.04% <86.04%> (ø)`	`15.00 <15.00> (?)`
...main/java/io/lettuce/core/RedisCommandBuilder.java	`94.96% <100.00%> (+0.09%)`	`472.00 <8.00> (+8.00)`
...ain/java/io/lettuce/core/protocol/CommandType.java	`100.00% <100.00%> (ø)`	`3.00 <0.00> (ø)`
.../io/lettuce/core/dynamic/ReactiveTypeAdapters.java	`88.26% <0.00%> (+0.43%)`	`1.00% <0.00%> (ø%)`
...java/io/lettuce/core/protocol/DefaultEndpoint.java	`70.06% <0.00%> (+0.46%)`	`99.00% <0.00%> (ø%)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe70d44...905d985. Read the comment docs.

mp911de · 2020-10-23T13:39:44Z

Thanks a lot, will have a look on Monday.

mp911de · 2020-10-26T10:59:34Z

Care to rebase your PR. Merging #1456 introduced some conflicts.

sokomishalov · 2020-10-26T12:22:43Z

Done

sokomishalov · 2020-11-01T23:38:42Z

src/main/templates/io/lettuce/core/api/RedisSortedSetCommands.java

+     * @param keys the keys.
+     * @return List&lt;V&gt; array-reply list of elements.
+     */
+    List<V> zunion(long numkey, ZStoreArgs storeArgs, K... keys);


My only concern on this - naming issue. We reuse similar ZStoreArgs but these commands even do not have "store" in their names. Shall we introduce just a wrapper class with new name which only extends it?

Reuse ZStoreArgs is fine to me. : ) . @mp911de Can you share you opinions for this?

ZStoreArgs doesn't really fit the new commands. While it was fine when we only had Z…STORE commands, the new variants require something better.

I'd suggest introducing ZAggregateArgs and retrofitting ZStoreArgs so that ZStoreArgs extends ZAggregateArgs. We don't need to deprecate anything. Related, introducing zunionstore(…, ZAggregateArgs, …) doesn't provide more value over zunionstore(…, ZStoreArgs, …), therefore we should keep the existing methods as-is.

@mp911de I do personally like the approach with introducing ZAggregateArgs where ZStoreArgs extends ZAggregateArgs.
But, unfortunately, we still have to ship to ZStoreArgs all builder methods (since they are statically dispatched) and override all setter methods to return ZStoreArgs. With current PR code, this code would not compile:

ZStoreArgs args = ZStoreArgs.Builder.max()

So I'm a bit confused of how to do it with neither API breaking changes nor tons of boilerplate.

dengliming

👍 . Can you add also @since tags (version 6.1) to all newly introduced public methods?

mp911de

The PR is a great starting point.

There are two general issues to address:

Public API methods (and their implementations) should be sorted alphabetically. We have a ton of methods already and alphabetical ordering makes it simple to find the methods in the code.
I'd suggest introducing ZAggregateArgs instead of reusing ZStoreArgs. See the comment below.

Let me know if there's anything unclear or to discuss.

src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.java

src/main/java/io/lettuce/core/api/async/RedisSortedSetAsyncCommands.java

src/main/java/io/lettuce/core/protocol/CommandType.java

mp911de · 2020-11-02T07:37:34Z

src/main/templates/io/lettuce/core/api/RedisSortedSetCommands.java

+     * @param keys the keys.
+     * @return List&lt;V&gt; array-reply list of elements.
+     */
+    List<V> zunion(long numkey, ZStoreArgs storeArgs, K... keys);


ZStoreArgs doesn't really fit the new commands. While it was fine when we only had Z…STORE commands, the new variants require something better.

I'd suggest introducing ZAggregateArgs and retrofitting ZStoreArgs so that ZStoreArgs extends ZAggregateArgs. We don't need to deprecate anything. Related, introducing zunionstore(…, ZAggregateArgs, …) doesn't provide more value over zunionstore(…, ZStoreArgs, …), therefore we should keep the existing methods as-is.

sokomishalov · 2020-11-06T13:16:55Z

👍 . Can you add also @since tags (version 6.1) to all newly introduced public methods?

Yep, my bad. Added.

mp911de · 2020-11-07T20:45:10Z

Going to take this PR from here.

Original pull request: #1469.

Revert zinterstore/zunionstore accepting ZAggregateArgs to retain binary compatibility. Remove numkey parameter as this parameter can be calculated from the number of specified keys to simplify API usage. Reinstate builder and override methods in ZStoreArgs to retain binary compatibility. Tweak Javadoc, add author tags and license headers. Fix Coroutines zmscore signature. Original pull request: #1469.

mp911de · 2020-11-07T20:59:41Z

Thank you for your contribution. That's merged and polished now. I especially reverted the changes for zunionstore/zinterstore to keep binary compatibility. Also, I removed numkey from the Java method signatures as we should determine the number of keys from the keys array size. That way, we prevent potential bugs when using the API.

sokomishalov force-pushed the feature/zinter-zunion branch from 9999703 to 492c8ae Compare October 26, 2020 12:22

sokomishalov force-pushed the feature/zinter-zunion branch 3 times, most recently from 0ef07d4 to ac87d7a Compare November 1, 2020 23:33

sokomishalov requested review from mp911de and dengliming and removed request for mp911de November 1, 2020 23:33

sokomishalov commented Nov 1, 2020

View reviewed changes

dengliming reviewed Nov 2, 2020

View reviewed changes

mp911de requested changes Nov 2, 2020

View reviewed changes

mp911de added this to the 6.1 M1 milestone Nov 2, 2020

mp911de added the type: feature A new feature label Nov 2, 2020

sokomishalov force-pushed the feature/zinter-zunion branch 4 times, most recently from a191fa5 to 3dfc469 Compare November 5, 2020 01:38

dengliming linked an issue Nov 5, 2020 that may be closed by this pull request

Add support for ZINTER/ZUNION commands #1450

Closed

mp911de self-assigned this Nov 5, 2020

sokomishalov force-pushed the feature/zinter-zunion branch from 3dfc469 to 7556ae1 Compare November 6, 2020 12:44

ZUNION/ZINTER commands introduced redis#1450

905d985

sokomishalov force-pushed the feature/zinter-zunion branch from 7556ae1 to 905d985 Compare November 6, 2020 13:11

mp911de pushed a commit that referenced this pull request Nov 7, 2020

ZUNION/ZINTER commands introduced #1450

9bf8491

Original pull request: #1469.

mp911de closed this Nov 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZUNION/ZINTER commands introduced #1469

ZUNION/ZINTER commands introduced #1469

sokomishalov commented Oct 23, 2020

codecov bot commented Oct 23, 2020 •

edited

Loading

mp911de commented Oct 23, 2020

mp911de commented Oct 26, 2020

sokomishalov commented Oct 26, 2020

sokomishalov Nov 1, 2020

dengliming Nov 2, 2020

mp911de Nov 2, 2020

sokomishalov Nov 6, 2020 •

edited

Loading

dengliming left a comment

mp911de left a comment

mp911de Nov 2, 2020

sokomishalov commented Nov 6, 2020

mp911de commented Nov 7, 2020

mp911de commented Nov 7, 2020

ZUNION/ZINTER commands introduced #1469

ZUNION/ZINTER commands introduced #1469

Conversation

sokomishalov commented Oct 23, 2020

codecov bot commented Oct 23, 2020 • edited Loading

Codecov Report

mp911de commented Oct 23, 2020

mp911de commented Oct 26, 2020

sokomishalov commented Oct 26, 2020

sokomishalov Nov 1, 2020

Choose a reason for hiding this comment

dengliming Nov 2, 2020

Choose a reason for hiding this comment

mp911de Nov 2, 2020

Choose a reason for hiding this comment

sokomishalov Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

dengliming left a comment

Choose a reason for hiding this comment

mp911de left a comment

Choose a reason for hiding this comment

mp911de Nov 2, 2020

Choose a reason for hiding this comment

sokomishalov commented Nov 6, 2020

mp911de commented Nov 7, 2020

mp911de commented Nov 7, 2020

codecov bot commented Oct 23, 2020 •

edited

Loading

sokomishalov Nov 6, 2020 •

edited

Loading