Skip to content

Improve readability and completedness for Redis - #9093

Merged
skjnldsv merged 7 commits into
nextcloud:masterfrom
martin-rueegg:patch-3
Mar 16, 2023
Merged

Improve readability and completedness for Redis#9093
skjnldsv merged 7 commits into
nextcloud:masterfrom
martin-rueegg:patch-3

Conversation

@martin-rueegg

Copy link
Copy Markdown
Contributor
  • give more complete examples for Redis config
  • use separate sections for the different connection types (TCP, TLS, socket)

Signed-off-by: Martin R眉egg martin.rueegg@metaworx.ch

- give more complete examples for Redis config
- use separate sections for the different connection types (TCP, TLS, socket)

Signed-off-by: Martin R眉egg <martin.rueegg@metaworx.ch>
@kesselb
kesselb requested review from blizzz and szaimen July 20, 2022 12:01
blizzz
blizzz previously requested changes Jul 22, 2022

@blizzz blizzz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great improvement, few nitpicks :)

Comment thread admin_manual/configuration_server/caching_configuration.rst
Comment thread admin_manual/configuration_server/caching_configuration.rst Outdated
Explain that using Redis for local cache is possible, but not recommended

Signed-off-by: Martin R眉egg <martin.rueegg@metaworx.ch>
@martin-rueegg

Copy link
Copy Markdown
Contributor Author

Thanks @blizzz for taking the time for reviewing this.

Comment thread admin_manual/configuration_server/caching_configuration.rst Outdated
Comment thread admin_manual/configuration_server/caching_configuration.rst Outdated
martin-rueegg and others added 5 commits September 2, 2022 23:05
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: Martin R眉egg <martin.rueegg@metaworx.ch>
Co-authored-by: Carl Schwan <carl@carlschwan.eu>
Signed-off-by: Martin R眉egg <martin.rueegg@metaworx.ch>
- replace "memcache" with "data cache" in the introduction to avoid confusion with "memcached"
- moving the "Recommendations" section to the top of the page (between intro and cache configuration)
- re-organize and expand the "Recommendations* section

Signed-off-by: Martin R眉egg <martin.rueegg@metaworx.ch>
Credits:  @2fst4u in #9125

Signed-off-by: Martin R眉egg <martin.rueegg@metaworx.ch>
Signed-off-by: Martin R眉egg <martin.rueegg@metaworx.ch>
@martin-rueegg

Copy link
Copy Markdown
Contributor Author

@nickvergessen, @blizzz, @CarlSchwan are you happy with the changes I made following your request?

@nickvergessen

Copy link
Copy Markdown
Member

I'm fine but I have little to no background to verify the content of your changes

@szaimen
szaimen requested review from PVince81 and skjnldsv October 17, 2022 19:28
@martin-rueegg

Copy link
Copy Markdown
Contributor Author

@PVince81 @skjnldsv , any chance to have a look at this, please?

@szaimen

szaimen commented Nov 8, 2022

Copy link
Copy Markdown
Contributor

@blizzz can you approve? :)

Comment thread admin_manual/configuration_server/caching_configuration.rst
Comment thread admin_manual/configuration_server/caching_configuration.rst
Comment thread admin_manual/configuration_server/caching_configuration.rst
@martin-rueegg

Copy link
Copy Markdown
Contributor Author

@PVince81 @szaimen @skjnldsv

All points raised by @blizzz regard information/sections, that have already been there before. I am not able to technically review the content of the entire document. But if additional changes are required then I suggest to proceed as follows:

  • If the change regards the formatting or layout or structure of the article, I'm happy to adjust those.
  • If the change regards the technical content, then a new PR/issue should be opened to address those.

Otherwise, we keep on the improvements in this PR from being published, and potentially helping users/admins. Just because the document should have been overhauled/redacted technically already.

What do you guys say?

@skjnldsv
skjnldsv dismissed blizzz鈥檚 stale review March 16, 2023 08:57

Addressed

@skjnldsv skjnldsv assigned skjnldsv and unassigned skjnldsv Mar 16, 2023
@skjnldsv skjnldsv added this to the Nextcloud 27 milestone Mar 16, 2023
@skjnldsv
skjnldsv merged commit 4a25e81 into nextcloud:master Mar 16, 2023
@martin-rueegg

Copy link
Copy Markdown
Contributor Author

Thanks, @skjnldsv.

@martin-rueegg
martin-rueegg deleted the patch-3 branch March 16, 2023 09:42
@skjnldsv

Copy link
Copy Markdown
Member

Thanks for your patience @martin-rueegg 馃

@ChristophWurst

Copy link
Copy Markdown
Member

I found

Using Redis for local cache on a multi-server setup can cause issues. Also, even on a single-server setup, APCu (see section above) should be faster.

for https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/caching_configuration.html#redis-configuration-in-nextcloud-config-php but not https://docs.nextcloud.com/server/25/admin_manual/configuration_server/caching_configuration.html. Does that mean with Nextcloud >= using Redis for local cache is not problematic? Otherwise I would suggest to backport the refinements of our recommendation.

@ChristophWurst

Copy link
Copy Markdown
Member

My remark is specifically about b502cc7. We can also only backport this one commit and leave the restructuring for 26+.

@martin-rueegg

martin-rueegg commented Mar 31, 2023

Copy link
Copy Markdown
Contributor Author

My remark is specifically about b502cc7. We can also only backport this one commit and leave the restructuring for 26+.

It's funny, because when I created this PR back in July last year, versions 22-24 where actively released. Don't think any of the examples I've added have been unavailable at that point. So why would it not be back ported to at least v24 or even before?

The note you mentioned was added due to @blizzz 's comment.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants