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 copy #1565

Closed

Conversation

KowalczykBartek
Copy link
Contributor

@KowalczykBartek KowalczykBartek commented Dec 21, 2020

Hi,
this is initial implementation for #1508 ,
I created as much I as could inferring from the codebase and already implemented commands, but I need some hints what else have to be added :)

@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #1565 (225002d) into main (6761a91) will decrease coverage by 0.25%.
The diff coverage is 53.84%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1565      +/-   ##
============================================
- Coverage     78.87%   78.62%   -0.26%     
- Complexity     6296     6340      +44     
============================================
  Files           470      476       +6     
  Lines         21044    21330     +286     
  Branches       2315     2333      +18     
============================================
+ Hits          16599    16771     +172     
- Misses         3380     3483     +103     
- Partials       1065     1076      +11     
Impacted Files Coverage Δ Complexity Δ
...va/io/lettuce/core/AbstractRedisAsyncCommands.java 95.80% <50.00%> (-0.19%) 433.00 <1.00> (+2.00) ⬇️
...io/lettuce/core/AbstractRedisReactiveCommands.java 88.75% <50.00%> (-0.14%) 423.00 <1.00> (+2.00) ⬇️
...main/java/io/lettuce/core/RedisCommandBuilder.java 94.80% <50.00%> (-0.21%) 476.00 <1.00> (+2.00) ⬇️
...ain/java/io/lettuce/core/protocol/CommandType.java 100.00% <100.00%> (ø) 3.00 <0.00> (ø)
...c/main/java/io/lettuce/core/ConnectionBuilder.java 66.40% <0.00%> (-20.62%) 31.00% <0.00%> (+4.00%) ⬇️
src/main/java/io/lettuce/core/KillArgs.java 75.00% <0.00%> (-15.00%) 11.00% <0.00%> (ø%)
...ce/core/resource/MappingSocketAddressResolver.java 59.09% <0.00%> (-9.34%) 5.00% <0.00%> (ø%)
.../java/io/lettuce/core/protocol/CommandWrapper.java 65.62% <0.00%> (-7.30%) 31.00% <0.00%> (-1.00%)
...o/lettuce/core/resource/SocketAddressResolver.java 68.18% <0.00%> (-6.82%) 7.00% <0.00%> (ø%)
.../java/io/lettuce/core/resource/KqueueProvider.java 16.98% <0.00%> (-5.47%) 2.00% <0.00%> (ø%)
... and 28 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 6761a91...9769eea. 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. Our API is generated from the templates at src/main/templates. If we have the change there, we can run our API generator during the merge if you want us to take over that part.

Other than that, we should introduce a CopyArgs type to encapsulate copy arguments.

}

@Override
public RedisFuture<Boolean> copy(K source, K destination, int destinationDd) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would make sense to introduce a CopyArgs object that includes destinationDb and replace options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't notice COPY has replace argument :) CopyArgs added (builder, comments etc.)

@@ -42,7 +42,7 @@

// Keys

DEL, DUMP, EXISTS, EXPIRE, EXPIREAT, KEYS, MIGRATE, MOVE, OBJECT, PERSIST, PEXPIRE, PEXPIREAT, PTTL, RANDOMKEY, RENAME, RENAMENX, RESTORE, TOUCH, TTL, TYPE, SCAN, UNLINK,
DEL, DUMP, EXISTS, EXPIRE, EXPIREAT, KEYS, MIGRATE, MOVE, OBJECT, PERSIST, PEXPIRE, PEXPIREAT, PTTL, RANDOMKEY, RENAME, RENAMENX, RESTORE, TOUCH, TTL, TYPE, SCAN, UNLINK, COPY,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: We typically order command types alphabetically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@KowalczykBartek
Copy link
Contributor Author

Thanks a lot for your comments :) I added also new signatures to src/main/templates ( I assumed COPY should be placed in RedisKeyCommands ). Do you have a task in CI that generates something based on that template ?

@mp911de mp911de added this to the 6.1 M1 milestone Jan 8, 2021
mp911de pushed a commit that referenced this pull request Jan 8, 2021
Original pull request: #1565.
mp911de added a commit that referenced this pull request Jan 8, 2021
Reorder methods and reorganize imports. Add Kotlin implementation.

Original pull request: #1565.
@mp911de mp911de added the type: feature A new feature label Jan 8, 2021
@mp911de mp911de self-assigned this Jan 8, 2021
@mp911de
Copy link
Collaborator

mp911de commented Jan 8, 2021

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

@mp911de mp911de closed this Jan 8, 2021
@KowalczykBartek
Copy link
Contributor Author

cool ! thank you for the review, going for the next one :)

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.

2 participants