Introduce JedisBatchOperationException #1733
Conversation
- Pipeline and Transaction related exceptions, which do not come from Redis, are changed to throw JedisBatchOperationException. - SafeEncoder is changed to throw JedisException. Based on discussions in redis#1183
sazzad16
force-pushed
the
confusing-JedisDataException
branch
from
7971f00 to
21125f0
Compare
|
@marcosnils @gkorland Please review this before releasing 3.0.0. This PR will remove wrongful usages of |
| @@ -0,0 +1,10 @@ | |||
| package redis.clients.jedis.exceptions; | |||
|
|
|||
| public class JedisBatchOperationException extends JedisException { | |||
There was a problem hiding this comment.
Can you make it extend "JedisDataException" this way it won't make a backward issue?
There was a problem hiding this comment.
@gkorland It was done deliberately.
According to marcosnils @marcosnils
I thought about this and I believe the best approach would be to change the type of the exception to
JedisExceptioninstead ofJedisDataExceptionand to improve the logging message. ... CurrentlyJedisDataExceptionis used mainly when Redis returns an error about the operation. The pipeline/transaction validation we're doing it's something from the Jedis side, so it doesn't make sense to return this type of exception. ...
According to HeartSaVioR @HeartSaVioR
... I also think we shouldn't use JedisDataException which is not ERR from Redis side. ...
I agree with both of them. And that's why I crafted this PR.
There was a problem hiding this comment.
i agree @gkorland advice, we should be compatible with JedisDataException, because maybe user code has catch JedisDataException.
So, the inheritance path is ： JedisBatchOperationException extends JedisDataException extends JedisException.
3 participants
Inspired by #1183 (comment) by @marcosnils and #1183 (comment) by @HeartSaVioR