-
Notifications
You must be signed in to change notification settings - Fork 269
RC: Active-Active improvments #2299
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
Conversation
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.
Mostly just some suggestions and easily-fixed typos, so I'll approve.
|
|
||
| {{<image filename="images/rc/subscription-add-region-throughput.png" alt="The Throughput step." >}} | ||
|
|
||
| You can also select **Set throughput as existing region** to duplicate the throughput settings from an existing region. |
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.
Not major, but maybe this line would look better above the image? It's a big image with a lot of white background, so this text looks more connected to the list item below.
|
|
||
| {{<image filename="images/rc/subscription-add-region-required-resources.png" alt="The Required resources step." >}} | ||
|
|
||
| Select **Continue** to add the region to your Active-Active deployment. |
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.
Separate list item, maybe? (Same issue as with the previous comment.)
| However, writes at times t4 and t6 are not concurrent as a sync happened | ||
| in between. | ||
|
|
||
| | **Time** | **CRDB Instance1** | **CRDB Instance2** | |
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 thought - do you think this might work well as a sequence diagram?
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.
Probably. I don't want to make that change without buy-in from @rrelledge since this appears in both the Redis Cloud and Redis Software docs, and might be slightly out of scope for this PR specifically.
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.
Sequence diagrams probably would be better than the current (very old) tables, but I think maybe that could be handled in a separate ticket/PR so that it doesn't block merging this one.
Co-authored-by: andy-stark-redis <[email protected]>
Section link:
https://redis.io/docs/staging/rc-active-active/operate/rc/databases/active-active/