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

Adds a clients overview page #166

Merged
merged 18 commits into from
Mar 3, 2025
Merged

Conversation

liorsve
Copy link
Contributor

@liorsve liorsve commented Nov 25, 2024

Description

Adds a client page to docs on the website. The page is added to the top nav docs, called "Client Libraries", and its url is top level valkey.io/clients .
This PR is dependent on the complimentary docs PR being merged.

How it works -

  • The init-topics.sh script was modified to expect the path to the clients docs folder as its second argument. It creates a symlink to it, called build-clients. The clients folder in docs holds json files specifying each client's details and features.
  • content/clients/_index.md has in its frontmatter the list of paths to the clients appearing in this page. This list is used by the client-list.html template to render the specified clients' data. This template also includes the client-feature-table.html template that renders the same json files to produce a table comparing which features each client implements.

Issues Resolved

#161
Dependent on this PR being merged - #164

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the BSD-3-Clause License.

Copy link
Member

@stockholmux stockholmux left a comment

Choose a reason for hiding this comment

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

It needs some adjustments and I have some general approach questions but quite good work on this!

One general comment that I couldn't stick anywhere: Make sure your final PR includes README instructions on how this works.

config.toml Outdated
@@ -48,4 +49,16 @@ publish_hold = [
"/topics/internals/",
"/topics/protocol/",
"/topics/rdd/"
]

recommended_clients_paths = [
Copy link
Member

Choose a reason for hiding this comment

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

Two questions here:

  • config.toml is a site-level configuration (e.g. included on every page). Is there a better place for this data? I would guess either the macro can load a arbitrary file or include a recommended flag in the JSON file itself. It could also be stored in custom frontmatter (see added custom frontmatter for topics, override for about/history #167), which might be a flexible alternative to present different recommendations on different pages.
  • How was the 'recommendation' list arrived at?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Alright @liorsve, we're dependent on #164 make sense.

@liorsve
Copy link
Contributor Author

liorsve commented Dec 1, 2024

  • cherry-picked added custom frontmatter for topics, override for about/history #167
  • added a custom frontmatter that overrides the default topics template, and contains the list of recommended clients' paths
  • created a new custom template for the clients page topic, which renders all client specific content from the json client files in valkey-doc/clients (github url, installation and description), and includes the feature comparison table.

@zuiderkwast
Copy link
Contributor

I fail to see in the diff what the URL of this page will be.

I suggest we put it under the root, at https://valkey.io/clients/. It doesn't really belong under topics.

We already have links to ../clients/ from topics and commands pages, because the corresponding page in the old redis website had this location too. By keeping this, we don't need to update the links in the doc repo.

I don't have a strong opinion about this though. If there is another plan, then we can just update the links in the docs to point to the right path, because I prefer that they make sense for doc writers.

@liorsve
Copy link
Contributor Author

liorsve commented Dec 18, 2024

@zuiderkwast I have no problem moving this, we haven't discussed it and I just put it in topics as a starting point for discussion, so we'll have some kind of visualization. Also it would simplify things if it's not under topics, cause then maybe I won't have to use the topic custom frontmatter PR. Where do you suggest placing this on the website?

Copy link
Member

@stockholmux stockholmux left a comment

Choose a reason for hiding this comment

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

This belongs in the top nav documentation drop down and on the /docs/ page as well. But the url should be top level (e.g. valkey.io/clients

@liorsve liorsve changed the title Adds a client feature comparison table for a future clients overview page Adds a clients overview page Jan 7, 2025
@liorsve liorsve force-pushed the website-clients-page branch from f8ee2d8 to 8e69fbc Compare January 9, 2025 13:55
@liorsve
Copy link
Contributor Author

liorsve commented Jan 9, 2025

Summarizing the big changes I added -

  1. The page moved from being a topic to the top nav docs, called "Client Libraries". With url valkey.io/clients.
  2. Because this is not a topic anymore and it requires creating a symlink to the clients folder in docs, it needs to be included in the shell script. But it's only for creating the symlink and it doesn't create any stub files like the current init-topics.sh does for topics, so it's a relatively little addition. So I though it's better to avoid adding a separate init-clients.sh file, and instead modify init-topics.sh to be init-topics-and-clients, and have it expect the path to topics as its first arg, and the path to clients as its second arg. README was updated accordingly.
  3. I moved here the markdown content for this page (i.e. explanations about the different client features), which previously was loaded from topics in the docs repo.
  4. Updated clients paths according to the change in structure of the clients folder in the docs repo PR - #164.

@pnbrown
Copy link
Contributor

pnbrown commented Jan 31, 2025

I'm not able to build this branch.

$user valkey-io.github.io % zola serve
Building site...
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 37 pages (0 orphan) and 6 sections
Error: Failed to serve the site
Error: Failed to render section '/Users/niigel/Repos/liorsve/valkey-io.github.io/content/clients/_index.md'
Error: Reason: Failed to render 'client-list.html'
Error: Reason: Function call 'load_data' failed
Error: Reason: `load_data`: /Users/niigel/Repos/liorsve/valkey-io.github.io/../build-clients//python/valkey-GLIDE.json doesn't exist

@pnbrown
Copy link
Contributor

pnbrown commented Jan 31, 2025

I see the additional instructions now. Disregard. Will try rebuilding

@pnbrown
Copy link
Contributor

pnbrown commented Jan 31, 2025

The page renders as if the right sidebar is there causing the text to wrap unnecessarily early and the table at the bottom of the page to be squashed. Is there anything that can be done about that?

Looks like this may also be an issue on the blogs.

Screenshot 2025-01-31 at 13 04 45 Screenshot 2025-01-31 at 13 06 55

@liorsve
Copy link
Contributor Author

liorsve commented Feb 9, 2025

The page renders as if the right sidebar is there causing the text to wrap unnecessarily early and the table at the bottom of the page to be squashed. Is there anything that can be done about that?

@pnbrown I updated it to use the fullwidth template, this is what it looks like now. I can also limit a bit only the width of the boxes since most of them don't use the full size anyway. I think the same should work for blogs with a few tweaks.
Screenshot 2025-02-09 at 13 08 46
Screenshot 2025-02-09 at 12 47 58

@liorsve liorsve marked this pull request as ready for review February 11, 2025 17:50
@liorsve liorsve requested a review from madolson as a code owner February 11, 2025 17:50
Signed-off-by: lior sventitzky <[email protected]>
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

looks pretty good


Selecting the right client is a complex task, given that there are over 200 clients compatible with Valkey across different programming languages. This document offers an overview of recommended Valkey clients for various programming languages. To be included in this list, a client must support a mandatory set of features, such as TLS support and cluster mode. Additionally, a table of advanced features supported by the respective clients is provided, highlighting the unique advantages of one client over another.

This page includes only clients which are regularly tested and recommended. However, it's important to note that all Redis and Valkey clients are compatible with Valkey 7.2 and above.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This page includes only clients which are regularly tested and recommended. However, it's important to note that all Redis and Valkey clients are compatible with Valkey 7.2 and above.

We can't guarantee this statement as written. I'm not really sure what we should say here. We should say other clients that support Redis OSS version 7.2 are supported maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to -
"However, it's important to note that other clients that support Redis OSS version 7.2 are compatible with Valkey 7.2 and above."
Is this ok?

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with that.


2. **TLS/SSL Support** - The capability to establish secure connections using TLS/SSL, which encrypts the data transmitted between the client and the server. This is a critical feature for applications that require data privacy and protection against eavesdropping.

Advanced Features Overview
Copy link
Member

Choose a reason for hiding this comment

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

Some of this seems duplicated from the valkey-doc client readme. Maybe we should have an advanced client feature topic and then import it here, so we aren't duplicating it? We can fast follow up with that if you agree, it's not a blocker to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean an actual topic in docs/topics? Or just dividing the docs/clients README into a separate client feature markdown in the same folder, that we will refer to from the docs README as well?
I think the second option is better, we already create a symlink to the clients docs repo from here so it would be easy to import, and there is no need to create a topic stub file that won't be used afterwards, and also having it as a topic in docs would just create another duplication there.
I can open a quick PR in the docs repo if you agree, or we can add this post-merge, whatever you prefer

Comment on lines 38 to 42
Mandatory Features Overview
----
1. **Cluster Support** - The ability to operate in a clustered environment, where the data is distributed across multiple shards. Cluster support is essential for applications that require high scalability.

2. **TLS/SSL Support** - The capability to establish secure connections using TLS/SSL, which encrypts the data transmitted between the client and the server. This is a critical feature for applications that require data privacy and protection against eavesdropping.
Copy link
Member

Choose a reason for hiding this comment

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

Did we decide this someplace? I remember this was in the original description of the PR, but I don't think we finalized on the set of mandatory features. I suppose I would drop this for now and keep the criteria vague until we get wider alignment.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should also include a link for how you can add your client to this list.

Copy link
Contributor Author

@liorsve liorsve Feb 28, 2025

Choose a reason for hiding this comment

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

Did we decide this someplace? I remember this was in the original description of the PR, but I don't think we finalized on the set of mandatory features. I suppose I would drop this for now and keep the criteria vague until we get wider alignment.

Will delete this, not sure if this was decided.

I think we should also include a link for how you can add your client to this list.

Added a link to docs/clients README at the intro paragraph, hope that was your intention?

liorsve and others added 2 commits February 28, 2025 12:54
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Looks good to me, I noticed that the instructions for valkey-glide go didn't look quite right, can we fix that before pushing it?

@pnbrown Do you have time to take another look?

{% endif %}
{% endfor %}
{% for language in languages %}
<li><a href="#{{ language | slugify }}">{{ language | title }}</a></li>
Copy link
Member

Choose a reason for hiding this comment

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

PHP is usually all capitalized, but that isn't properly captured here. Not sure if I really care though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • valkey glide- go installation instructions fix PR for docs repo - Clients: Fix Glide-go& Glide-java installation, update valkey-go version valkey-doc#241
  • Fixed PHP capitalization in the new commit.
  • I also changed the sass style for code width, in the docs PR I added a fix for the glide-java maven and gradle installation which caused it to be wider. After this fix, Gradle installation for Glide now looks like this with a scroll bar -
    Screenshot 2025-03-02 at 10 46 38
    But the line too long to fit in the box anyway so I don't see a good way to avoid it.

Copy link
Contributor

@pnbrown pnbrown left a comment

Choose a reason for hiding this comment

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

LGTM

@madolson madolson merged commit 88b9469 into valkey-io:main Mar 3, 2025
1 check passed

7. **Client Side Caching** - Valkey client-side caching is a feature that allows clients to cache the results of Valkey queries on the client-side, reducing the need for frequent communication with the Valkey server. This can significantly improve application performance by lowering latency, reducing the network usage and cost and reducing the load on the Valkey server.

8. **`CLIENT CAPA redirect` Support** - The `CLIENT CAPA redirect` feature was introduced in Valkey 8 to facilitate seamless upgrades without causing errors in standalone mode. When enabled, this feature allows the replica to redirect data access commands (both read and write operations) to the primary instance. This ensures uninterrupted service during the upgrade process. For more detailed information about this feature, please refer to [this link.](https://github.com/valkey-io/valkey/pull/325)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are multiple links to PRs.

This is a mistake. We should link to documentation, not to PRs.

Copy link
Member

Choose a reason for hiding this comment

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

@liorsve Can you take a look?

I was okay referencing PRs, but I'm fine point to actual documentation too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's not a disaster. It's just better to link to the docs. The docs are user facing. The PRs are developer facing.

Copy link
Contributor Author

@liorsve liorsve Mar 5, 2025

Choose a reason for hiding this comment

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

@madolson @zuiderkwast
I'll open a new PR - we can refer to docs to explain about client capa from the server side, but to replace the link explaining about AZ awareness the only relevant docs I found is the blog post about it that's mainly focused on Glide, is that ok with you? If not I think we can also omit it, it's not such a big deal.
Also, if we want to follow up with the multiplication you pointed out here at the same time, we can open that PR directly in the docs repo, and then update things here so that all of this content would come from the docs repo directly. LMK if my suggestion there in the comment seems acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the docs are not very good for this feature. We can keep the link to the PR until we have improved the documentation.

Here is what I found and some ideas:

  • We have some documentation of the INFO field availability_zone in https://valkey.io/commands/info/ and in the HELLO response in https://valkey.io/commands/hello/
  • We don't have the configs in the documentation. This is something I want to have and there is an issue for it: #Document configs valkey-doc#154
  • We have been talking loosely about creating a document about client best practices where we could describe how clients should behave in various situations and maybe also how they should implement features like this one.
  • We should probably document reading from replicas properly in https://valkey.io/topics/replication/ or in some other page. The replication page mentions it but it doesn't explain much about it: "Replication can be used both for scalability, to have multiple replicas for read-only queries (for example, slow O(N) operations can be offloaded to replicas), or simply for improving data safety and high availability."

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.

5 participants