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 doc page on Redis #1015

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Add doc page on Redis #1015

wants to merge 13 commits into from

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Sep 5, 2024

previews: GitHub, RTD

Description of proposed changes

Preparing for upgrade (https://github.com/nextstrain/private/issues/121)

Currently, the GitHub markdown preview reflects my intentions. I might convert to RST instead so things render nicely on RTD.

Checklist

Start by moving the existing content from the infrastructure page.
Make this distinctively about minor maintenance tasks and set
expectations.
The rename is apparent on the add-on page, presumably because of the
shift to using Valkey with version 7.2.¹

¹ https://devcenter.heroku.com/changelog-items/2918
Reduce the amount of first-person wording which made sense for the note
form, but not so much in a proper guide.
This can be detected automatically. The same can likely be done with the
new instance name (to replace redis-X-N), but I don't want to create a
new instance to test this at the moment.
This is necessary to test session restoration.
@victorlin victorlin self-assigned this Sep 5, 2024
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-victorlin--pt6y6j September 5, 2024 21:40 Inactive
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--pt6y6j September 5, 2024 21:51 Inactive
@victorlin
Copy link
Member Author

victorlin commented Sep 5, 2024

Not ready to be merged, but the content (GitHub preview) is ready for review.

I'll wait until after doing the 6.2→7.2 upgrade to see if there are any additional details to include before merging.

@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--pt6y6j September 5, 2024 21:58 Inactive
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

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

A few minor suggestions, none blocking. Thanks for doing this work!

+1 from me for converting to rST and cleaning it up for rendering on RTD too.

Comment on lines +83 to +84
as all data looks to be transferred. Do this by entering Redis CLI (`heroku
redis:cli`) on both instances and comparing the output of:
Copy link
Member

Choose a reason for hiding this comment

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

heroku redis:cli used to not connect over TLS, which was bonkers, so for a long time I've used my own little program wrapping the Heroku CLI and standard Redis CLI.

But I just checked again now, and at least as of heroku version

heroku/7.60.2 linux-x64 node-v14.19.0

it does indeed connect over TLS.

I think if you want to use more advanced features of the Redis CLI, you'll still need to invoke it yourself directly (heroku redis:cli --help doesn't indicate it will pass thru any args), but it's nice that for a simple REPL you can just use heroku redis:cli now.

Comment on lines +87 to +89
- [`scan`](https://redis.io/docs/latest/commands/scan/) (start with `scan 0`
and follow the cursor a couple times)
- a manually issued [`sync`](https://redis.io/docs/latest/commands/sync/) jumping over bulk sync and right to live monitor
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
- [`scan`](https://redis.io/docs/latest/commands/scan/) (start with `scan 0`
and follow the cursor a couple times)
- a manually issued [`sync`](https://redis.io/docs/latest/commands/sync/) jumping over bulk sync and right to live monitor
- [`scan`](https://valkey.io/commands/scan/) (start with `scan 0`
and follow the cursor a couple times)
- a manually issued [`sync`](https://valkey.io/commands/sync/) jumping over bulk sync and right to live monitor

Comment on lines +27 to +33
> [!NOTE]
> Heroku provides an [in-place upgrade
> method](https://devcenter.heroku.com/articles/heroku-redis-version-upgrade#upgrade-using-redis-upgrade)
> that is much simpler than the steps below. However, there is no option to roll
> back in case anything unexpected happens. If we find ourselves going through
> this process more often without any failures and this becomes too tedious, it
> may be worth switching to use the in-place upgrade method.
Copy link
Member

Choose a reason for hiding this comment

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

Linking back to our discussion of the tradeoffs here (https://github.com/nextstrain/private/issues/121#issuecomment-2330682764) will probably be helpful in the future.

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.

3 participants