-
Notifications
You must be signed in to change notification settings - Fork 16
fix: read/write splitting switch to writer #1080
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
9217b12 to
f3c0000
Compare
| conn.read_only = False | ||
|
|
||
| writer_id = rds_utils.get_cluster_writer_instance_id() | ||
| new_member = writer_id |
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.
why do we assign writer_id to new_member? the following lines are actually clearer if you keep the name writer_id?
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.
Good point, I just deleted the code path from the if else condition as the original, will change it to improve readability
| # We should not be able to switch again because new_member was removed from the custom endpoint. | ||
| # We are connected to the reader. Attempting to switch to the writer will throw an exception. |
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.
To clarify:
- reconnected back to the original reader instance on line 277
- removed the original reader instance from the custom endpoint
- now switching to the writer instance (a valid member of the endpoint) errors out
This is because the current reader connection is invalid? Wouldn't any operation fail?
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:
- Connected to reader node, readOnly=False should fail since writer is not in allowed list
- Add writer and then switch to Writer so it should be good,
- Switch back to reader (readOnly=True), then remove writer (not original reader)
- Now try to switch to Writer and it should fail.
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'll add something like this in the beginning of each test so it's more descriptive, and will change the variable names
f3c0000 to
349ef26
Compare
Co-authored-by: Sophia Chu <112967780+sophia-bq@users.noreply.github.com>
Description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.