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

Add support for ZADD GT/LT options #1455

Closed
wants to merge 2 commits into from

Conversation

dengliming
Copy link
Contributor

@dengliming dengliming commented Oct 10, 2020

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

closes #1451

@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #1455 into main will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1455      +/-   ##
============================================
- Coverage     78.92%   78.84%   -0.09%     
  Complexity     6187     6187              
============================================
  Files           460      460              
  Lines         20800    20814      +14     
  Branches       2302     2304       +2     
============================================
- Hits          16417    16411       -6     
- Misses         3324     3342      +18     
- Partials       1059     1061       +2     
Impacted Files Coverage Δ Complexity Δ
src/main/java/io/lettuce/core/ZAddArgs.java 100.00% <100.00%> (ø) 12.00 <2.00> (+4.00)
...io/lettuce/core/protocol/ChannelLogDescriptor.java 57.14% <0.00%> (-21.43%) 4.00% <0.00%> (-1.00%)
.../lettuce/core/masterreplica/SentinelConnector.java 65.85% <0.00%> (-17.08%) 6.00% <0.00%> (-2.00%)
...in/java/io/lettuce/core/masterreplica/Timeout.java 62.50% <0.00%> (-12.50%) 3.00% <0.00%> (-1.00%)
.../io/lettuce/core/protocol/ReconnectionHandler.java 79.72% <0.00%> (-4.06%) 22.00% <0.00%> (ø%)
...e/core/masterreplica/SentinelTopologyProvider.java 81.08% <0.00%> (-2.71%) 9.00% <0.00%> (-1.00%)
...ce/core/masterreplica/SentinelTopologyRefresh.java 84.96% <0.00%> (-1.97%) 35.00% <0.00%> (ø%)
...a/io/lettuce/core/protocol/ConnectionWatchdog.java 80.39% <0.00%> (-1.31%) 39.00% <0.00%> (-1.00%)
src/main/java/io/lettuce/core/ScanStream.java 86.53% <0.00%> (-1.29%) 24.00% <0.00%> (ø%)
...java/io/lettuce/core/protocol/DefaultEndpoint.java 70.06% <0.00%> (-0.93%) 99.00% <0.00%> (-1.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 d3d5054...abfb421. Read the comment docs.

Copy link
Collaborator

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

Thanks a lot, great work. Can you add also @since tags (version 6.1) to all newly introduced public methods in ZAddArgs and yourself as @author?

I think we also need a guard to the @Test methods when running the tests against an older Redis version, but I currently don't have a good idea. Maybe it's time to introduce @EnabledOnRedisVersion. Happy for suggestions.

src/main/java/io/lettuce/core/ZAddArgs.java Show resolved Hide resolved
@dengliming
Copy link
Contributor Author

dengliming commented Oct 12, 2020

Maybe it's time to introduce @EnabledOnRedisVersion.

@mp911de Great idea. Maybe we should track this with separated issue? Seems that we can get the current redis version by calling RedisConditions.getRedisVersion WDYT?

@mp911de
Copy link
Collaborator

mp911de commented Oct 26, 2020

For now, we can guard the tests with @EnabledOnCommand("ZMSCORE") since ZMSCORE was added in Redis 6.2, too.

@mp911de mp911de added this to the 6.1 M1 milestone Oct 26, 2020
@mp911de mp911de added the type: feature A new feature label Oct 26, 2020
@dengliming
Copy link
Contributor Author

Thanks. will update it later.

@mp911de
Copy link
Collaborator

mp911de commented Oct 26, 2020

No worries, I'm about to merge and polish up this PR.

mp911de pushed a commit that referenced this pull request Oct 26, 2020
mp911de added a commit that referenced this pull request Oct 26, 2020
Guard tests so they run only with Redis 6.2. Reformat code.

Original pull request: #1455.
@mp911de
Copy link
Collaborator

mp911de commented Oct 26, 2020

Thank you for your contribution. That's merged and polished now.

@mp911de mp911de closed this Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for ZADD GT/LT options
2 participants