Skip to content

feat: Support Leader Aware Routing#78

Merged
SandeepTuniki merged 17 commits into
mainfrom
lar-with-better-tests
Jan 31, 2024
Merged

feat: Support Leader Aware Routing#78
SandeepTuniki merged 17 commits into
mainfrom
lar-with-better-tests

Conversation

@SandeepTuniki

@SandeepTuniki SandeepTuniki commented Jan 23, 2024

Copy link
Copy Markdown
Contributor

This PR implements Leader Aware Routing (LAR) for the Ruby Spanner client. For background design, see http://shortn/_5ecDWu2XD9 (internal)

Note:

  • LAR is enabled by default. But we provide an option to users to disable it during client initialization.
  • Admin RPCs in the handwritten Ruby client are deprecated, so LAR isn't supported for these RPCs.

Tests

  • Majority of the testing is done through unit tests.
  • For acceptance tests, a basic smoke test is added to validate querying without LAR. All the existing acceptance tests run with LAR enabled by default.

@SandeepTuniki SandeepTuniki marked this pull request as ready for review January 23, 2024 13:00
@SandeepTuniki SandeepTuniki requested review from a team January 23, 2024 13:00
@SandeepTuniki SandeepTuniki changed the title feat: Implement Leader Aware Routing feat: Support Leader Aware Routing Jan 23, 2024
# limitations under the License.


module Google

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure why do we need a separate place to handle this as the context for calling these methods are still with the respective methods why not just default the values in place itself ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mostly to facilitate future changes easily. I first used the default values in place. But soon I found that these "magical constants" were spread across the codebase, making it harder to spot any specific values in the future. So I moved them to this helper module.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that's fine. However, the downside of doing this is that these values are separated from their context so it's less clear what they are for. So I recommend making sure we document each of the methods here with what the value is used for (and maybe under what circumstances it might change in the future).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added description at the top of the module explaining the context, and when the values can differ.

I avoided commenting for individual methods to avoid duplication. But let me know if the present documentation doesn't suffice. I'll make changes.

@dazuma dazuma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few minor comments, otherwise looks good.

Comment thread google-cloud-spanner/lib/google/cloud/spanner/lar_headers.rb Outdated
# limitations under the License.


module Google

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that's fine. However, the downside of doing this is that these values are separated from their context so it's less clear what they are for. So I recommend making sure we document each of the methods here with what the value is used for (and maybe under what circumstances it might change in the future).

@SandeepTuniki SandeepTuniki merged commit f3cf5e1 into main Jan 31, 2024
@SandeepTuniki SandeepTuniki deleted the lar-with-better-tests branch January 31, 2024 06:13
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