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

fixed bug in batched pipeline #288

Merged
merged 5 commits into from
Nov 2, 2021
Merged

fixed bug in batched pipeline #288

merged 5 commits into from
Nov 2, 2021

Conversation

debasishg
Copy link
Owner

When sending commands in batch the Format related to the arguments were not being sent over. Fixed it.

() => client.hmset("hash1", Map("field1" -> Upper("val1"), "field2" -> Upper("val2"))),
() => client.hmset("hash2", Map("field1" -> Upper("val1"), "field2" -> Upper("val2")))(formatUpper),
() => client.hmget("hash1", "field1", "field2"),
() => client.hmget("hash2", "field1", "field2")
Copy link

Choose a reason for hiding this comment

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

This doesn't check the return value of hmget - is the assumption that as long as it executes with error, it is correct (i.e. there is test coverage for formatting and parsing elsewhere)?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The return values are all returned at once in batch. Please check the test cases in PipelineSpec

@debasishg
Copy link
Owner Author

@noahlz I pushed the fix - had to switch to binary protocol for the batching. Also updated test cases. Please let me know if this works.

@debasishg
Copy link
Owner Author

@noahlz Added all the test cases from #290 into PipelineSpec and all run fine. Let me know if things look good for you. I will do some cleanups and have another release.

@noahlz
Copy link

noahlz commented Nov 2, 2021

Ok, thanks - will test shortly. In the meantime, should we close and delete the branch for #290 ?

@noahlz
Copy link

noahlz commented Nov 2, 2021

I'm working through a problem where ZADD does not insert strings. I suspect because my predecessor in our application eschewed the default format and parse implicits.

I.e. we have a wrapper classes RedisPool and RedisFunctions that I suspect are Doing It Wrong (this is closed source, snippets below).

redisPool.forPipeline("test") {
  List((cmd: com.redis.RedisCommand) => RedisFunctions.ssetAdd("my key", ttl = None, "foo bar", "bazz bar")(cmd))
}

and then

scala> redisPool.withClient("test") { client => client.zrange("my key", 0, -1) }
res1: Option[List[String]] = Some(List(??bazz ba�, ??foo ba�))

scala> res2.get.foreach(println)
bazz ba�
foo ba�

In redis cli:

> zrange "mykey" 0 -1
1) "\x03\x01bazz ba\xf2"
2) "\x03\x01foo ba\xf2"

I'm working through this to figure out what's wrong. Otherwise, your changes look good. I added test case to try to reproduce the issue on my side and could not, so clearly we are Not Using Format and Parse Implicits Correctly.

@debasishg debasishg merged commit 83dbcfc into master Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants