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 Redis adapter #37

Open
wants to merge 12 commits into
base: 3.x
Choose a base branch
from
Open

Add Redis adapter #37

wants to merge 12 commits into from

Conversation

vamsiikrishna
Copy link

No description provided.

@lyrixx
Copy link
Collaborator

lyrixx commented Sep 22, 2015

First thanks for your contribution.

As there is many redis adapater in the wild (predis credis, php_redis), could you rename the Redis class to CRedis ?

@vamsiikrishna
Copy link
Author

@lyrixx
I had similar idea, but I don't think creating a new class for every redis adapter will be practical .
any other approach you can recommend ?

Thanks

@lyrixx
Copy link
Collaborator

lyrixx commented Sep 22, 2015

I don't see any others.

@vamsiikrishna
Copy link
Author

@lyrixx ok, will rename redis class to CRedis for now.

I will keep searching for any other ways.
btw all redis libraries just need host and port to initiate the connection, that's why I asked if there is a better way.

Thanks

@vamsiikrishna
Copy link
Author

Hi @lyrixx
I have made necessary change . Please review

Thanks

class CRedis implements Collector
{
private $host;
private $port;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you never use $host, and $port, no need to save it.

@lyrixx
Copy link
Collaborator

lyrixx commented Oct 22, 2015

@vamsiikrishna Do you have time to finish your PR ?

{
$this->credis->incr($variable);
}

Copy link

Choose a reason for hiding this comment

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

@inheritdoc should be added on methods

@vamsiikrishna
Copy link
Author

@lyrixx I will try to complete this during weekend .

Regards

@vamsiikrishna
Copy link
Author

@lyrixx @stof I have added support for sf bundle and have tried to implement the changes you guys have suggested. Please review .

Thanks

@vamsiikrishna
Copy link
Author

Hi @lyrixx ,

any chance you can review it again ?
thanks

@lyrixx
Copy link
Collaborator

lyrixx commented Nov 25, 2015

@vamsiikrishna sure. But there is still pending comment that should be addressed. After that, we will be OK ;)

@vamsiikrishna
Copy link
Author

@beberlei sorry to bother you again
I had done the necessary changes , but I do have some doubts about the way I have implemented the bundle.
can you please check this ?

*/
public function increment($variable)
{
$this->credis_client->incr($variable);
Copy link

Choose a reason for hiding this comment

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

https://redis.io/commands/incr/

Note: this is a string operation because Redis does not have a dedicated integer type. The string stored at the key is interpreted as a base-10 64 bit signed integer to execute the operation.

Once you reach the limit of 2 pow 63 + 1, you will get an error

https://stackoverflow.com/questions/36861472/what-happens-when-int64-maxvalue-is-exceded-with-redis-incr

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.

4 participants