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

feat: async factory function for eager connections #535

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

cprice404
Copy link
Contributor

This commit makes the following changes:

  • Stop trying to do the eager connection stuff in the constructor
    of the CacheClient, because this can have surprising side effects,
    e.g. if we want to throw an error on failure.
  • Remove the associated Configuration settings since we no longer
    support eager connections at construction time.
  • Add a new factory function, CacheClient.CreateAsync, which is the
    new mechanism for create a client with an eager connection. This
    matches the pattern we established in the other SDKs that support
    eager connections.
  • If the eager connection fails, we now throw a new ConnectionError
    rather than just logging a warning. This gives the consumer
    the ability to decide how to handle this type of error rather than
    us just swallowing it and removing their choice. In most cases,
    users would just end up hitting a timeout error on their next
    request after we gave up on the eager connection.

In the future we may add more configuration options for how to
handle eager connection failures, possibly including some
automatic retries. For now, this gives the user more control, which
seems extremely desirable given the number of times we have recently
seen users run into DNS throttling when they try to make a very
high volume of connections to Momento from lambdas.

@cprice404
Copy link
Contributor Author

I have tested this by overriding the cache server DNS name on the Configuration object, setting it to a bad/invalid hostname so that the DNS lookup fails. It behaves as desired in those conditions. I haven't found a way to test the actual DNS throttling scenario yet; it seems like it will require spinning up 10k lambdas so I don't know that we should block the PR on that.

malandis
malandis previously approved these changes Feb 23, 2024
Copy link
Contributor

@malandis malandis left a comment

Choose a reason for hiding this comment

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

@malandis
Copy link
Contributor

These tests will need to be modified/removed/skipped until later.

This commit makes the following changes:

* Stop trying to do the eager connection stuff in the constructor
  of the CacheClient, because this can have surprising side effects,
  e.g. if we want to throw an error on failure.
* Remove the associated Configuration settings since we no longer
  support eager connections at construction time.
* Add a new factory function, CacheClient.CreateAsync, which is the
  new mechanism for create a client with an eager connection. This
  matches the pattern we established in the other SDKs that support
  eager connections.
* If the eager connection fails, we now throw a new ConnectionError
  rather than just logging a warning. This gives the consumer
  the ability to decide how to handle this type of error rather than
  us just swallowing it and removing their choice. In most cases,
  users would just end up hitting a timeout error on their next
  request after we gave up on the eager connection.

In the future we may add more configuration options for how to
handle eager connection failures, possibly including some
automatic retries. For now, this gives the user more control, which
seems extremely desirable given the number of times we have recently
seen users run into DNS throttling when they try to make a very
high volume of connections to Momento from lambdas.
@cprice404 cprice404 merged commit 2324880 into main Feb 23, 2024
7 checks passed
@cprice404 cprice404 deleted the make-eager-connection-factory-fn branch February 23, 2024 06:39
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.

2 participants