Update lettuce to 3.x #104

mp911de · 2014-09-18T18:30:01Z

Update lettuce redis client to latest snapshot, adopt changed API and use API for commands which were emulated using LUA script
Implement Sentinel support using lettuce library, adopt finer grained exceptions and aligned exception behavior to match other libraries
Added shutdown-timeout property in order to control shutdown duration of the netty framework which reduces test run duration
Documentation updated
Contains also a fix to use psetex instead of setex (typo)
Switched MinimumRedisVersionRule to Jedis since Jedis has a much faster shutdown behavior than lettuce

Update lettuce redis client to latest snapshot, adopt changed API and use API for commands which were emulated using LUA script Reference: DATAREDIS-348

Implement Sentinel support using lettuce library, adopt finer grained exceptions and aligned exception behavior to match other libraries. Added shutdown-timeout property in order to control shutdown duration of the netty framework which reduces test run duration. Documentation updated as well. Contains also a fix to use psetex instead of setex (typo). Reference: DATAREDIS-348

mp911de · 2014-09-18T18:30:42Z

Please review this PR. It is in the first step a proposal how to adopt lettuce 3.x into spring-data-redis.

christophstrobl · 2014-09-25T07:30:51Z

src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java

-	private Object await(Command cmd) {
-		if (isMulti && cmd.type != MULTI) {
+	private Object await(com.lambdaworks.redis.protocol.RedisCommand cmd) {
+		if (isMulti && cmd instanceof Command && ((Command)cmd).isMulti()) {


should have been && !((Command)cmd).isMulti() right?

right. It's a typo and I no test fails. This seems awkward to me. I'll add a test case for this behavior

I took a look again since the code was confusing. await(...) is called only in regular mode (pipeline=false and queuing=false). This means isMulti is in await(...) always false.

Another issue that I noticed is:

Issuing MULTI via exec on a regular connection (connection.execute("MULTI")) with await(...) will return an empty List. All following commands will be triggered within the MULTI on Redis but won't provide a result on the final EXEC. This is because lettuce queues all MULTI-related outputs in an internal
MultiOutput. The MultiOutput in case of a EXEC using execute(...) is empty and has no definition about the data structures that come back.

So coming back to await(...) it means that the if-clause can be removed safely.

Does it make sense?

christophstrobl · 2014-09-25T08:07:56Z

thx @mp911de that looks pretty decent - as this is quite a big shift I'd ask you to give us a little time to think things through in detail.

mp911de · 2014-09-25T08:18:54Z

Sure. I'll provide the changes mentioned above. Take as long as you need. If you need any changes to lettuce, come back to me.

thomasdarimont · 2014-11-14T14:00:06Z

Hi Mark,

would you mind rebasing this on the current master? I wanted to give it a spin - but I couldn't apply your patch.

Cheers,
Thomas

Btw. I just read on your site that you're from Weinheim - I'm often near Viernheim - let me know if you want to grab a beer ;-)

mp911de · 2014-11-14T14:53:04Z

Hi Thomas,
I'll do so. Guess, you'll need a new PR then, right? Hit me up if you're in the area.

thomasdarimont · 2014-11-14T15:55:19Z

Hi,
I think it already works of you just update and push the branch again that you used to issue the PR.
http://stackoverflow.com/questions/7947322/preferred-github-workflow-for-updating-a-pull-request-after-code-review

Great looking forward to meet you :)

Conflicts: src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionIntegrationTests.java

mp911de · 2014-11-14T17:45:59Z

Merged master into DATAREDIS-348. I also created a rebased branch (https://github.com/mp911de/spring-data-redis/tree/issue/DATAREDIS-348b, see https://github.com/mp911de/spring-data-redis/compare/issue/DATAREDIS-348b for comparison) since I'm not quite sure wether the merge results in what we expect.

mp911de · 2015-05-19T20:24:02Z

@christophstrobl @thomasdarimont still interested in the lettuce 3.x upgrade or should I abandon this PR?

thomasdarimont · 2015-05-19T21:11:46Z

Hi Mark,

sorry for the radio silence - we worked on different projects for a while and Christoph currently is and I will be on holiday for the next 2 weeks.
Would you be so kind to rebase this PR again on the latest SD Redis master?
I just tried to apply the PR as a patch but it failed due to some code shifting here and there that took place in the meantime :-(.

Although we focus on using the jedis driver in SD Redis internally, it would be helpful to have an updated lettuce alternative at hand, especially given that the lettuce driver we currently use was last updated in 2013...
Since we still have Lettuce support in SD Redis and your fork is the most active and well maintained (and comes with support for Redis cluster) I'd vote to definitely upgrade to this.

@christophstrobl @olivergierke thoughts?

Cheers,
Thomas

odrotbohm · 2015-05-20T12:53:24Z

I am not too much in the details but it worries me that this PR seems to introduce a lot of dependencies to the Lattice driver in types that didn't have driver dependencies before. Is that by purpose?

mp911de · 2015-05-20T13:15:05Z

I'm not quite sure which types you're talking about. Do you mean netty or the massive change within the import sections?

I could not find types in which imports to com.lambdaworks.redis were added and that did not contain previously imports to com.lambdaworks.redis.

odrotbohm · 2015-05-20T13:26:53Z

Looks like the imports are ordered differently. They showed additions but the removals further below (which I oversaw). All good.

…DATAREDIS-348c Conflicts: gradle.properties src/asciidoc/reference/redis.adoc src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java

mp911de · 2015-06-02T16:55:57Z

Rebased this PR to the most recent master. Added support for HLL and ZRANGEBYLEX

christophstrobl · 2015-06-02T18:03:15Z

thanks @mp911de!

Updated lettuce redis client to latest snapshot, adopt changed API and use API for commands which were emulated using LUA script Implemente Sentinel support using lettuce library, adopt finer grained exceptions and aligned exception behavior to match other libraries. Added shutdown-timeout property in order to control shutdown duration of the netty framework which reduces test run duration. Documentation updated as well. Contains also a fix to use psetex instead of setex (typo). Switch to Jedis for Redis Version discovery in tests. Polished documentation. Use LettuceConverter and native time() method. Original pull request: #144. Related pull request: #104.

Fixed connection problems leaving sockets open. Added missing JUnit rules for Sentinels. Added missing author information update, license header and Javadoc. Upgraded test script to use Redis 3.0.2. Upgrade test script to shut down Redis in case of test errors. Added Redis 3.0 to TestProfileValueSource. Enabled ZRANGEBYLEX tests for lettuce. Original pull request: #144. Related pull request: #104.

mp911de · 2015-09-02T19:52:23Z

This PR could be closed, lettuce 3.0 was merged to master with #144. WDYT?

christophstrobl · 2015-09-03T10:54:07Z

fixed in #144

mp911de added 7 commits September 12, 2014 22:28

Update lettuce to 3.x

62cf2bf

Update lettuce redis client to latest snapshot, adopt changed API and use API for commands which were emulated using LUA script Reference: DATAREDIS-348

Fix typo

ea81c18

Polish documentation

a8d248b

Switch to Jedis for Redis Version discovery in tests

c20336b

Polishing of documentation

58d1c22

Cleanups after lettuce 3.0.1 release and clean code polishing

3383ab9

christophstrobl reviewed Sep 25, 2014
View reviewed changes

Use LettuceConverter and native time() method

d20a78b

christophstrobl force-pushed the master branch from f807f76 to f6a7ec5 Compare October 23, 2014 06:44

Merge branch 'master' into issue/DATAREDIS-348

011a154

Conflicts: src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionIntegrationTests.java

mp911de added 5 commits May 23, 2015 18:10

Update to lettuce 3.2.Beta1

915c4f6

Merge remote-tracking branch 'origin/issue/DATAREDIS-348' into issue/…

11781d6

…DATAREDIS-348c Conflicts: gradle.properties src/asciidoc/reference/redis.adoc src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java

Merge remote-tracking branch 'origin/issue/DATAREDIS-348' into issue/…

1433512

…DATAREDIS-348c Conflicts: gradle.properties src/asciidoc/reference/redis.adoc src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java

Add support for HLL and ZRANGE commands in lettuce

300b78a

Merge remote-tracking branch 'upstream/master' into issue/DATAREDIS-348c

66d4238

Rebase to spring-data-redis/master

3e40a25

christophstrobl mentioned this pull request Jun 8, 2015

DATAREDIS-348 - Switch to mp911de/lettuce 3.0. #144

Closed

christophstrobl closed this Sep 3, 2015

spring-projects-issues mentioned this pull request Dec 30, 2020

Upgrade Lettuce to 3.x release [DATAREDIS-348] #896

Closed

Update lettuce to 3.x #104

Update lettuce to 3.x #104

Uh oh!

Conversation

mp911de commented Sep 18, 2014

Uh oh!

mp911de commented Sep 18, 2014

Uh oh!

christophstrobl Sep 25, 2014

Choose a reason for hiding this comment

Uh oh!

mp911de Sep 25, 2014

Choose a reason for hiding this comment

Uh oh!

mp911de Sep 26, 2014

Choose a reason for hiding this comment

Uh oh!

christophstrobl commented Sep 25, 2014

Uh oh!

mp911de commented Sep 25, 2014

Uh oh!

thomasdarimont commented Nov 14, 2014

Uh oh!

mp911de commented Nov 14, 2014

Uh oh!

thomasdarimont commented Nov 14, 2014

Uh oh!

mp911de commented Nov 14, 2014

Uh oh!

mp911de commented May 19, 2015

Uh oh!

thomasdarimont commented May 19, 2015

Uh oh!

odrotbohm commented May 20, 2015

Uh oh!

mp911de commented May 20, 2015

Uh oh!

odrotbohm commented May 20, 2015

Uh oh!

mp911de commented Jun 2, 2015

Uh oh!

christophstrobl commented Jun 2, 2015

Uh oh!

mp911de commented Sep 2, 2015

Uh oh!

christophstrobl commented Sep 3, 2015

Uh oh!

Uh oh!