-
Notifications
You must be signed in to change notification settings - Fork 52
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
Introduce Valkey client overview #164
Conversation
asafpamzn
commented
Aug 17, 2024
- Created a new document that provides an overview of recommended Valkey clients across various programming languages.
- Included mandatory features required for each client, such as Cluster Support and TLS/SSL Support.
- Detailed advanced features supported by the clients, including:
- Read from Replica
- Exponential Backoff to Prevent Storm
- Valkey Version Compatibility
- PubSub State Restoration
- Cluster Scan
- Latency-Based Read from Replica
- Client-Side Caching
- Added feature comparison tables for each programming language (Python, JavaScript/Node.js, Java, Go, PHP, C#) to highlight the unique capabilities of each client.
- Placeholder sections for Ruby and other languages marked as TODO for future updates.
- References section includes a link to the official Valkey documentation.
@asafpamzn Thank you for that initiative. it was discussed several times that we need some feature support metric for known clients. Several high level remarks:
|
@ranshid , thanks for the feedback.
|
@ranshid , @zuiderkwast , I updated the PR according to your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the summary overall. @rueian @yangbodong22011 Would also be great if you can review this.
I think we can put all clients
|
I like this idea. If the table is large, it can be messy to maintain it as Markdown. If we have one JSON file per client (like we have for the old redis clients still in this repo), we could add this information in the JSON files and render the table on the website. |
agree👍 |
I don't agree, usually customers are first choosing the language and next the client, the client is a small part at the application, basically a driver. I don't think that the average reader will care about clients in other languages. I do agree that for the valkey maintainers it is a better view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to decide where to put this document.
- Created a new document that provides an overview of recommended Valkey clients across various programming languages. - Included mandatory features required for each client, such as Cluster Support and TLS/SSL Support. - Detailed advanced features supported by the clients, including: - Read from Replica - Exponential Backoff to Prevent Storm - Valkey Version Compatibility - PubSub State Restoration - Cluster Scan - Latency-Based Read from Replica - Client-Side Caching - Added feature comparison tables for each programming language (Python, JavaScript/Node.js, Java, Go, PHP, C#) to highlight the unique capabilities of each client. - Placeholder sections for Ruby and other languages marked as TODO for future updates. - References section includes a link to the official Valkey documentation. Signed-off-by: asafpamzn <[email protected]>
Co-authored-by: Avi Fenesh <[email protected]> Signed-off-by: asafpamzn <[email protected]>
…#159) Update cluster-slots docs ref: valkey-io/valkey#265 --------- Signed-off-by: Roshan Khatri <[email protected]> Signed-off-by: Madelyn Olson <[email protected]> Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: asafpamzn <[email protected]>
Add documentation for the new `info stats` fields. --------- Signed-off-by: Uri Yagelnik <[email protected]> Signed-off-by: asafpamzn <[email protected]>
In connection to #164, and as per @zuiderkwast's suggestion, this PR deletes all of the old Redis clients files and leaves the clients folder empty. This is to allow for changing this folder's structure to be `clients/{language}/{client}.json` and adding new json files for the new clients that would serve a clients docs page in the website, as described in #164. Signed-off-by: lior sventitzky <[email protected]>
Looks bad for the site or for the client? |
"latency_based_read_from_replica": false, | ||
"AZ_based_read_from_replica": false, | ||
"client_side_caching": false, | ||
"client_capa_redirect": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"client_capa_redirect": false, | |
"client_capa_redirect": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 👍
"github":"https://github.com/valkey-io/valkey-go", | ||
"installation": "go get github.com/valkey-io/valkey-go", | ||
"language":"go", | ||
"package_size": "14.5M", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree remove package_size field, this is what package download sites like maven and pypi should do.
Looks bad both for clients and website IMO, it can look less aesthetic in the table if it's a super long list of features, and also just make the impression that one client has significantly less supported features than another even if it's not the case. But if we're more or less aligned there it can work. Besides that about uniqueness, we can always use the |
Signed-off-by: lior sventitzky <[email protected]>
Signed-off-by: lior sventitzky <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments on the READMEs. Looks good in general.
Signed-off-by: lior sventitzky <[email protected]>
clients/go/valkey-go.json
Outdated
"repo":"https://github.com/valkey-io/valkey-go", | ||
"installation": "go get github.com/valkey-io/valkey-go", | ||
"language":"go", | ||
"package_size": "14.5M", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think package_size
should be removed. For Go, package_size
doesn't seem very useful to users. The portion of the library in the final binary depends not only on the library version but also on how the user uses it and compiles the program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is also package_size won't really be actively maintained at all, so it will also quickly fall out of date. I think we should omit it.
Co-authored-by: Rueian <[email protected]> Signed-off-by: Lior Sventitzky <[email protected]>
Signed-off-by: lior sventitzky <[email protected]>
I think we're also supporting removing the package size field, and if it's also less relevant for some languages, sounds like regardless of the question of whether we want to include version data, package size should be removed. @valkey-io/client-maintainers Pinging everyone once again to address the remaining open issues so we can advance towards merging this -
|
For both 1&2 I support adding the max information, as long as its easy for a user to see if the information is out of date, for example by adding a "entry was last checked & updated on " field. |
@nihohit I suggested earlier that we put the release date of the version described. If we put "entry was last updated", it may even become outdated while a change is waiting to be merged. We'll end up with "last updated 2025-01-28" merged on 2025-02-10 in the commit log. Another way would be to pick the date from git. Not sure if our website generator can do that though. |
clients/php/predis.json
Outdated
"name": "predis", | ||
"description": "A flexible and feature-complete Redis client for PHP.", | ||
"twitter": [ | ||
"JoL1hAHN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm the new owner for the last couple of years.
"JoL1hAHN" | |
"tillkruss" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this field is actually a leftover from the old structure of these JSON files, and so it won't be rendered anywhere when this goes to the website. I removed this for all of the other JSON client files as well and left it here by mistake, so I think It's better to just remove this field altogether :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Let's let the github be the main access page, and then individuals can include links like twitter there.
Co-authored-by: Till Krüss <[email protected]> Signed-off-by: Lior Sventitzky <[email protected]>
Co-authored-by: Till Krüss <[email protected]> Signed-off-by: Lior Sventitzky <[email protected]>
Signed-off-by: lior sventitzky <[email protected]>
3. **`installation`** - an installation command from the most used package manager in the respective language. | ||
4. **`language`** - the programming language in which the library is written. | ||
5. **`package_size`** - the library's unpacked package size, including dependencies. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that credentials providers integration should also be part of the features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would ask we should probably stop adding features here, and just get the current list up to date. This PR has been open for awhile, and it's probably for the best to get it merged.
clients/php/predis.json
Outdated
{ | ||
"name": "predis", | ||
"description": "A flexible and feature-complete Redis client for PHP.", | ||
"repo":"https://github.com/predis/predis", | ||
"installation": "composer require predis/predis", | ||
"language":"php", | ||
"package_size": "2.8M", | ||
"license": "MIT", | ||
"read_from_replica": true, | ||
"smart_backoff_to_prevent_connection_storm": true, | ||
"pubsub_state_restoration": false, | ||
"cluster_scan": false, | ||
"latency_based_read_from_replica": false, | ||
"AZ_based_read_from_replica": false, | ||
"client_side_caching": false, | ||
"client_capa_redirect": false, | ||
"persistent_connection_pool": false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any change you can add Relay as well?
{
"name": "Relay",
"description": "Next-generation caching layer for Valkey/Redis.",
"repo": "https://github.com/cachewerk/relay",
"installation": "https://relay.so/docs/installation",
"language": "php",
"package_size": "2.5M",
"license": "Proprietary",
"read_from_replica": true,
"smart_backoff_to_prevent_connection_storm": true,
"pubsub_state_restoration": false,
"cluster_scan": false,
"latency_based_read_from_replica": false,
"AZ_based_read_from_replica": false,
"client_side_caching": true,
"client_capa_redirect": false,
"persistent_connection_pool": true
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proprietary? I'd say no to that. We do open source only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main repo seems to be using an MIT license, is it actually proprietary?
Signed-off-by: lior sventitzky <[email protected]>
@zuiderkwast @madolson I removed package_size, and added version and version date. So updated view of how this looks like - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Can you merge the latest main into this branch?
We should mention the latest changes in the client maintainers meeting. We discussed it last time and some things have changed since then.