making Jedis compatible with CDI

[nykolaslima]

When using CDI and trying to produce a Jedis and JedisPool we got an exception because CDI needs default constructor to make the object proxy.

Here is some project showing the problem: https://github.com/nykolaslima/cdiProblem

This change creates default constructor for Jedis and JedisPool classes. This constructor should only be used by CDI, thats why I added a @Deprecated.

[nykolaslima]

@HeartSaVioR and @marcosnils can you guys take a look, please?

[HeartSaVioR]

@nykolaslima How about creating another classes for mocking/proxying purposes?
Extending Jedis may works.
I'm -1 on leaving deprecated but not planned to remove.

[nykolaslima]

Hello Jungtaek.

I think that extends classes to make then compatible with CDI it's
overdesign.

The deprecated annotation is just to tell users that this constructor
should only be used by CDI. In my opinion, this is the simplest and less
intrusive way to make Jedis compatible with CDI.

If we extend classes, users will need to use different classes only because
of CDI, this will make it harder to use and the documentation harder to
make.

On Saturday, September 20, 2014, Jungtaek Lim notifications@github.com
wrote:

@nykolaslima https://github.com/nykolaslima How about creating another
classes for mocking/proxying purposes?
Extending Jedis may works.
I'm -1 on leaving deprecated but not planned to remove.

—
Reply to this email directly or view it on GitHub
#747 (comment).

[HeartSaVioR]

@nykolaslima I think Jedis / JedisPool with default constructor is not really usable for normal situation, so it should not exposed whether it has @deprecated or not.
Instead of using @deprecated, we should find a way that works with normal situation.
Marking @deprecated and describing it from comment is never enough. Comment isn't seen from auto-complete with IDE, just show this method is deprecated, and other users will never use this method.
Please don't think workaround until there're no way to do it.

[nykolaslima]

Thanks for reply.

Actually this is not usable for normal situation, it's just usable for CDI,
that's why we are marking it as deprecated.
We hope that CDI finds a better way to work and no longer depends on
default constructors. Until that's not done, many projects are using this
approach to work with CDI.
Create a new class is an overdesign, because when CDI no longer depends on
default constructors, we can delete these new classes and users that use
them will broke. Adding default constructor, users can keep using Jedis the
same way they used too and when CDI no longer depends on default
constructors, we can just remove them.

The number of people using CDI is increasing every day and do not
supporting it is really bad for Jedis.

Again, thank you for your replies. I undestand your point but I trully
believe that this is the best solution for this problem.

On Saturday, September 20, 2014, Jungtaek Lim notifications@github.com
wrote:

@nykolaslima https://github.com/nykolaslima I think Jedis / JedisPool
with default constructor is not really usable for normal situation, so it
should not exposed whether it has @deprecated
https://github.com/Deprecated or not.
Instead of using @deprecated https://github.com/Deprecated, we should
find a way that works with normal situation.
Marking @deprecated https://github.com/Deprecated and describing it
from comment is never enough. Comment isn't seen from auto-complete with
IDE, just show this method is deprecated, and other users will never use
this method.
Please don't think workaround until there're no way to do it.

—
Reply to this email directly or view it on GitHub
#747 (comment).

[HeartSaVioR]

@nykolaslima Here is working example about CDI and "current" Jedis.
http://xicojunior.wordpress.com/2013/09/03/simple-crud-using-servlet-3-0-redisjedis-and-cdi-part-2/

Seems like Jedis can play with CDI, except directly mocking/proxying Jedis & JedisPool.

[nykolaslima]

Hello Jungtaek,

Did you saw the example project that I created? Try to run it and you will
see that Jedis does not play with CDI.

In the example used in this blog post, the Jedis producer does not declare
the request scope. This causes the problem that more than one Jedis object
will be created in the same request. E.g. The same request could open 3
redis connection when it should only open one.

If you don't declare the producer as request scoped, it will work. But if
you want to make it request scoped, it wont work because CDI cannot proxy
Jedis class because it dont have the default constructor(you can see in the
example project that I posted)

On Sunday, September 21, 2014, Jungtaek Lim notifications@github.com
wrote:

@nykolaslima https://github.com/nykolaslima Here is working example
about CDI and "current" Jedis.

http://xicojunior.wordpress.com/2013/09/03/simple-crud-using-servlet-3-0-redisjedis-and-cdi-part-2/

Seems like Jedis can play with CDI, except directly mocking/proxying Jedis
& JedisPool.

—
Reply to this email directly or view it on GitHub
#747 (comment).

[nykolaslima]

@HeartSaVioR can you tell me if you guys gonna merge it? Thank you.

[HeartSaVioR]

@nykolaslima We recently release 2.6.0 with hard working to review and merge (including by hand).
So I need to take a break. Sorry about it.
Furthermore, I'm a Spring person so I don't know CDI yet.
Last, I don't accept @deprecated, so we should find a normal usage with default constructor.

[HeartSaVioR]

@marcosnils @xetorthio Can you handle this PR?

[marcosnils]

@HeartSaVioR. I'll check it out. I've been without Internet the past week
but now I'm online.

I'll let u know my thoughts soon
El sep 25, 2014 12:13 AM, "Jungtaek Lim" notifications@github.com
escribió:

@marcosnils https://github.com/marcosnils @xetorthio
https://github.com/xetorthio Can you handle this PR?

—
Reply to this email directly or view it on GitHub
#747 (comment).

[HeartSaVioR]

@nykolaslima @marcosnils @xetorthio
We can make default constructor of Jedis / JedisPool to use default host, "localhost" which may be meaningful that doesn't need to mark @deprecated.
With default constructor we can support mocking / proxying of Jedis / JedisPool.
What do you think about it?

[nykolaslima]

Looks good to me. I'll change it and commit.

On Saturday, September 27, 2014, Jungtaek Lim notifications@github.com
wrote:

@nykolaslima https://github.com/nykolaslima @marcosnils
https://github.com/marcosnils @xetorthio https://github.com/xetorthio
We can make default constructor of Jedis / JedisPool to use default host,
"localhost" which may be meaningful that doesn't need to mark @deprecated
https://github.com/Deprecated.
With default constructor we can support mocking / proxying of Jedis /
JedisPool.
What do you think about it?

—
Reply to this email directly or view it on GitHub
#747 (comment).

[HeartSaVioR]

@nykolaslima Default port is specified to Connection class.
So I recommend you to follow current style instead of passing localhost directly from Jedis.
Jedis, BinaryJedis, Client, BinayClient, Connection also can have default constructor.

[nykolaslima]

@HeartSaVioR can you please take a look now?

[HeartSaVioR]

@nykolaslima It seems that Connection class can handle default value, and other classes can delegate it to Connection class.
What do you think?

[HeartSaVioR]

I'm 👍 on this.
My comments are trivial but related to style. So I think it can merged into master without applying my comment.

[nykolaslima]

So do you want me to make the changes that you suggested or let's merge
like it is now?

2014-09-27 15:33 GMT-03:00 Jungtaek Lim notifications@github.com:

I'm [image: 👍] on this.
My comments are trivial but related to style. So I think it can merged
into master without applying my comment.

—
Reply to this email directly or view it on GitHub
#747 (comment).

[HeartSaVioR]

@nykolaslima First, it needs to reviewed by one more collaborator to merge.
I think that applying my comments are necessary to unify style, but others (including you) can think it isn't.
So I wish to hear others' thought about it.

[nykolaslima]

Ok. For me there is no problem in make these changes. But I think that's ok
now.

Let's wait for the review of the other guys. Thank you for helping me until
now.

Regards,
Nykolas

On Saturday, September 27, 2014, Jungtaek Lim notifications@github.com
wrote:

@nykolaslima https://github.com/nykolaslima First, it needs to reviewed
by one more collaborator to merge.
I think that applying my comments are necessary to unify style, but others
(including you) can think it isn't.
So I wish to hear others' thought about it.

—
Reply to this email directly or view it on GitHub
#747 (comment).

[HeartSaVioR]

@nykolaslima Thanks for your patience!

[HeartSaVioR]

@xetorthio @marcosnils Could you take a look and comment?

[nykolaslima]

Hey guys, do I need to make anything else so we can merge it?

[HeartSaVioR]

@nykolaslima It seems that other committers are very busy to take a look. I'll take care of it alone.
Could you apply my comments to unify style?

Btw, it'll be released to 2.7.0 (minor release), means it doesn't scheduled to release.

[nykolaslima]

@HeartSaVioR I did the changes that you suggested. Can we merge now?

[HeartSaVioR]

@nykolaslima I'm sorry, but you're misunderstanding my comments.

We let Connection class takes care of default port alone.
Other classes "should not assign" port when they want to use default port. They should not know what default port's value is.

We can apply this approach to default host.

But I can take care of it myself. I'll merge your PR into master, and modify it.

[nykolaslima]

I'm sorry Jungtaek.

Thank you for making this changes.

[]s

On Wednesday, October 8, 2014, Jungtaek Lim notifications@github.com
wrote:

@nykolaslima https://github.com/nykolaslima I'm sorry, but you're
misunderstanding my comments.

We let Connection class takes care of default port alone.
Other classes "should not assign" port when they want to use default port.
They should not know what value default port is.

We can apply this approach to default host.

But I can take care of it myself. I'll merge your PR into master, and
modify it.

—
Reply to this email directly or view it on GitHub
#747 (comment).

[marcosnils]

@HeartSaVioR. Thanks for merging. As you' ve stated I' ve been quite busy
the last weeks and I haven't been able to look at Jedis PR or issues.
Hopefully, some time this week or next one at most I' ll jump again into
the project.
On Oct 8, 2014 8:08 PM, "Jungtaek Lim" notifications@github.com wrote:

Merged #747 #747.

—
Reply to this email directly or view it on GitHub
#747 (comment).

[HeartSaVioR]

I've merged to master, and 2.7 branch.

[HeartSaVioR]

@marcosnils No problem! Feel free to take a break or do your own job first. :)

[nykolaslima]

Thank you @HeartSaVioR