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

Expand Client with a method for sending arbitrary commands. #395

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

martinnj
Copy link
Contributor

@martinnj martinnj commented Apr 27, 2022

This is the first step towards implementing #87.

The ElastiCache configuration command returns a multi-line reply that the current clients are incapable of reading fully, so a new method for reading the response from the socket is needed.

But since some of the required code for ElastiCache is a bit low in the client stack I thought it best to get some eyes on that part before doing the AWS specific stuff.

The reason I put it as a public method on Client is because any other client will need this one to send the configuration request to ElastiCache before being able to create the actual server configuration.

I have some questions/notes:

  • Currently if end_tokens is not found in the buffer, the client will hang forever, or timeout, with a real socket connection. For the unit-test we just get IndexError. What would be considered good behavior? The issues with the misc function is that it puts a lot of trust in the knowledge of the user. How much responsibility should/can the client take?
  • In the base misc implementation, I to some encoding shenanigans, that sets the encoding for every misc call, should that maybe be done in the constructor?
  • For pooled client, implementation is trivial, but for HashClient the semantics of a misc command is less clear, for instance, should a command be send to all clients, or should the command be hashed like a key, and sent to a specific client? The "optimal" outcome depends on what kind of command it is.
    My immediate thought is to just hash it and send it to a specific client, but I would like to hear your thoughts.

Please let me know what you think, and if I've completely gone off the rails somewhere.

@martinnj martinnj force-pushed the feature/elasticache branch 2 times, most recently from 453810f to 4b2364f Compare April 27, 2022 18:19
@martinnj
Copy link
Contributor Author

Looks like the failing test is realted to some SSL certs that have expired. I don't believe it's related to the committed code? :)

@martinnj martinnj marked this pull request as ready for review April 27, 2022 18:24
@jogo
Copy link
Contributor

jogo commented May 16, 2022

Looking into the cert failure first: #396

Copy link
Contributor

@jogo jogo left a comment

Choose a reason for hiding this comment

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

This is the first step towards implementing #87.

The ElastiCache configuration command returns a multi-line reply that the current clients are incapable of reading fully, so a new method for reading the response from the socket is needed.

But since some of the required code for ElastiCache is a bit low in the client stack I thought it best to get some eyes on that part before doing the AWS specific stuff.

Thanks for the PR!

The reason I put it as a public method on Client is because any other client will need this one to send the configuration request to ElastiCache before being able to create the actual server configuration.

I have some questions/notes:

  • Currently if end_tokens is not found in the buffer, the client will hang forever, or timeout, with a real socket connection. For the unit-test we just get IndexError. What would be considered good behavior? The issues with the misc function is that it puts a lot of trust in the knowledge of the user. How much responsibility should/can the client take?

Hmm I think the warnings in the docstrings are good enough.

  • In the base misc implementation, I to some encoding shenanigans, that sets the encoding for every misc call, should that maybe be done in the constructor?

Working under the assumption the misc commands won't be called super often, I think leaving it in the function for ease of use sounds good to me.

  • For pooled client, implementation is trivial, but for HashClient the semantics of a misc command is less clear, for instance, should a command be send to all clients, or should the command be hashed like a key, and sent to a specific client? The "optimal" outcome depends on what kind of command it is.
    My immediate thought is to just hash it and send it to a specific client, but I would like to hear your thoughts.

Two possible options:

  • Add a flag to support both options.
  • Hash it etc. and just make sure it's well documented and we can revisit as needed in the future,.

Please let me know what you think, and if I've completely gone off the rails somewhere.

Going to ask for a second pair of eyes on this, but looks good to me (minus one tiny typo)

pymemcache/client/base.py Outdated Show resolved Hide resolved
@jogo
Copy link
Contributor

jogo commented Jun 1, 2022

A rebase should fix the unit test failure too

pymemcache/client/base.py Outdated Show resolved Hide resolved
pymemcache/client/base.py Outdated Show resolved Hide resolved
pymemcache/client/base.py Outdated Show resolved Hide resolved
martinnj added a commit to martinnj/pymemcache that referenced this pull request Jun 7, 2022
martinnj added a commit to martinnj/pymemcache that referenced this pull request Jun 7, 2022
jparise pushed a commit that referenced this pull request Jun 7, 2022
Basically ensure the client only does one pass over the buffer instead of two.
Exact thread: #395 (comment)
@martinnj martinnj marked this pull request as draft June 8, 2022 11:14
@martinnj
Copy link
Contributor Author

martinnj commented Jun 8, 2022

I'm going to try and clean up some of the mess I made in git, while adding the discussed changes. Converted to draft while I do this.

martinnj added a commit to martinnj/pymemcache that referenced this pull request Jun 9, 2022
…est#402)

Basically ensure the client only does one pass over the buffer instead of two.
Exact thread: pinterest#395 (comment)
@martinnj martinnj marked this pull request as ready for review June 9, 2022 15:28
@martinnj
Copy link
Contributor Author

martinnj commented Jun 9, 2022

I have added the discussed changes and expanded testing a bit.

I seem to have made a mess of the branch though. And Honestly not sure how to fix it.
I can create a new branch and cherry-pick & squash the relevant commits if you feel like that is neater than this.

That would mean the communication in this PR gets lost though.

@jparise
Copy link
Collaborator

jparise commented Jun 9, 2022

I seem to have made a mess of the branch though. And Honestly not sure how to fix it. I can create a new branch and cherry-pick & squash the relevant commits if you feel like that is neater than this.

A little git rebase -i master on your branch followed by a force-push should work.

That would mean the communication in this PR gets lost though.

It happens. 🙂

@martinnj
Copy link
Contributor Author

martinnj commented Jun 9, 2022

I seem to have made a mess of the branch though. And Honestly not sure how to fix it. I can create a new branch and cherry-pick & squash the relevant commits if you feel like that is neater than this.

A little git rebase -i master on your branch followed by a force-push should work.

Neat git magic performed. Ritual was a success. Thanks for the tip! :)

Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

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

Looks good!

Sends an arbitrary command to the server and parses the response until a
specified token is encountered.

OBS: If the end_tokens are not included in the reply, the client will wait until it's
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what OBS means?

Copy link
Contributor Author

@martinnj martinnj Jun 9, 2022

Choose a reason for hiding this comment

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

That might be a Danish-ism I let slip in. Just short of "observe" could also be read as "NOTE" or "take note of".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, we often see "NB" (Nota bene) in the US instead.

Let's avoid using a shorthand either way. :)

Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

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

I thought a little more about this, and I don't think that send_to_all is the best approach.

If you're using HashClient, the expectation for all of the other commands is that you'll be routed to a single ("random") host. The stats command isn't available via HashClient for this reason.

I think the send_to_all option should be removed, and if the primary use case for raw_command is to target a specific host (or all hosts), then HashClient shouldn't have raw_command.

Separately, we could consider whether a "send to all" or "broadcast" system makes sense for HashClient (or any pool of clients).

`raw_command` allows for sending raw commands to Memcached, and specify a custom end-token.
This allows for communicating with non-standard servers such as ElasticCache instances.

Part of implementing pinterest#87.
@martinnj
Copy link
Contributor Author

martinnj commented Jun 9, 2022

If you're using HashClient, the expectation for all of the other commands is that you'll be routed to a single ("random") host. The stats command isn't available via HashClient for this reason.

I don't think it's as clear cut. flush_all, quit and close are all available on the HashClient and get propagated to every registered host, if live/up. The idea was to allow for similar style of calls to be made through raw_command if necessary.

With that said, I have no strong feelings, and this is not gonna get in the way of the ElastiCache client, so I'm fine with just axing it. to make sure the APIs are easily understood and do not foster misunderstandings about what they do.

@jparise jparise merged commit d0c180d into pinterest:master Jun 9, 2022
@jparise
Copy link
Collaborator

jparise commented Jun 9, 2022

Thanks!

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