Skip to content
This repository was archived by the owner on Oct 22, 2019. It is now read-only.

Add Predis support#55

Open
fesor wants to merge 3 commits into
Jimdo:masterfrom
fesor:predis_support
Open

Add Predis support#55
fesor wants to merge 3 commits into
Jimdo:masterfrom
fesor:predis_support

Conversation

@fesor

@fesor fesor commented Sep 18, 2017

Copy link
Copy Markdown

For #52

@fesor

fesor commented Nov 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

@schnipseljagd @bracki guys, what do you think on this?

@bracki

bracki commented Nov 6, 2017

Copy link
Copy Markdown
Contributor

I'd prefer to refactor the class so that it takes either a Redis or a Predis $connection, rather than duplicating everything. @fesor what's your take on that?

@fesor

fesor commented Nov 6, 2017

Copy link
Copy Markdown
Author

Will try to do that.

@fesor
Introduce redis client adapter
e2d1074 
 - Encapsulate differences between predis and redis behind
single abstraction.
 - Reduce duplication in tests
@fesor

fesor commented Feb 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Author

@bracki so, I need this again so I finally managed to work on this PR.

I added additional abstraction which hides difference between redis and predis without sacrifice lazy connection in case of redis.

I really not sure it is ok or not.

@bracki

bracki commented Mar 19, 2018

Copy link
Copy Markdown
Contributor

@fesor Would you mind rebasing? Did you also take into account #27?

bracki
bracki reviewed Mar 19, 2018
View reviewed changes
Comment thread src/Prometheus/PushGateway.php
* @param $groupingKey
*/
public function delete($job, $groupingKey = null)
public function delete(CollectorRegistry $collectorRegistry, $job, $groupingKey = null)

@bracki bracki Mar 19, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$collectorRegistry is left out on purpose here. It's not being used anyways. Modeled after the Python implementation which also leaves out the registry when doing a delete.

@fesor

fesor commented Mar 20, 2018

Copy link
Copy Markdown
Author

@bracki as for #27, yes and no. You can pass redis instance but adapter still will handle connection and will require connection options.

I need to check is this covers #27 fully.

Willl rebase this PR asap.

@peterjumpnl

peterjumpnl commented Sep 25, 2018

Copy link
Copy Markdown

What is status of this pr?

@MyIgel

MyIgel commented Jun 26, 2019

Copy link
Copy Markdown

Is there anything I could help with this PR to get merged? This would be a really nice feature :(

@NoelDavies

NoelDavies commented Oct 20, 2019

Copy link
Copy Markdown

This project is dead, but I'm maintaining it under my employer - https://github.com/endclothing/prometheus_client_php. Feel free to submit the PR there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@bracki bracki bracki left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fesor @bracki @peterjumpnl @MyIgel @NoelDavies