Skip to content

Conversation

@xslogic
Copy link

@xslogic xslogic commented Aug 11, 2012

Hello Kevin

I have added basic glob support for taba names. All the queries, except '/raw' can now accept a glob. (I havn't been able to test /raw, it keeps giving me an internal server error.. will check it out soon)
Assumptions/Observations:

  • I guess even though the server and dao supports a list of names as an input parameter, from reading the request handler code, It looks like the user either gives a single Taba name or none at all. (This could possibly me extended to support comma separated list of names)
  • Currently, to check if the user has provided a glob, I just check if the taba name found in the request contains special characters "*?[]!". It is possible that if the name contains an escaped character, this check will break. (Maybe it would be good to specify that only alphanumeric taba names are allowed ??)

This is just an initial patch.. let me know if you need me to clean it up further or if im missing something

Regards
-Arun Suresh

[Update] I had accidentally sent you the pull request before pushing to my repo. You need to apply both the patches in the same order now
[Update] Removed gevent dependency from taba_client

Copy link
Owner

Choose a reason for hiding this comment

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

"str" is a built-in function. Please don't override it.

@kevinballard
Copy link
Owner

For the client, I think we should have two versions -- one gevent compatible, and one that uses threading. This is because the two are incompatible (if you use gevent, you shouldn't use threading). Each version should have it's own module.

For the glob filters, I would not push that down to redis_engine. The filtering is an application level operation, so I would do the actual filtering in varz_server, or maybe varz_server_storage_manager.

Instead of looking for special characters in the "name" of "client" query parameter, I would use entirely different query parameters, so that's it's unambiguous.

-Kevin

P.S.: It is possible to pass in a list of Varz Names to retrieve by POSTing them to the *_batch handlers.

@xslogic
Copy link
Author

xslogic commented Aug 14, 2012

For the client.. I agree, I will create two separate versions of the client.

I do understand that glob filtering is an application level operation, but in the event that you have a large number of taba names, it might be slightly more efficient to let each shard return a filtered list..

I can introduce a 'taba_glob' paramter.. but we could end up with situations where the user provides a list of taba names as input as well as a taba_glob value such that some/all the names provided in the list do not match the glob. This could lead to some confusion / un-expected response from what the user might expect. Or else we could say that a user might specific either a glob OR name(s). What do u think ?

By the way, do you need glob support for both client_id as well as taba name ?

@kevinballard
Copy link
Owner

Since the Taba Server uses gevent (and not truly parallel threading), I don't think you're going to see any performance benefit to doing the filtering at the engine level. That engine is also shared by other projects, so I would like to avoid introducing Taba-specific logic to it.

I think it's reasonable to enforce either a list of names or a glob, but not both. The Taba Server Handlers already do that type of enforcement on parameters.

Support for glob queries on Client IDs would be nice as well.

* Removed glob filtering from engine
* Created separate client package for gevent and threaded clients
@xslogic
Copy link
Author

xslogic commented Aug 20, 2012

Hey Kevin

  • Moved all the glob filtering to the handler and server. GET queries now support a "taba_glob" parameter (only one of "taba_glob" or the "taba" parameter can be specified) and a "client_glob" parameter (only one of "client_glob" or "client_id" can be specified).
  • Made a couple of changes to some methods on the taba_server_storage_handler to support client list
  • if the "names" or "clients" list is empty, earlier, it used to be treated as if the value is None and all names were returned. Now, if the input list is empty, a null list will be returned (this is to support cases where none of the taba names or clients match the provided glob)
  • There are now versions of "taba_client" each in its own package.

Cheers
-Arun

@kevinballard
Copy link
Owner

Arun,

The approach looks good. If you feel up to it, you could generalize the "Param" class to contain all the common parameters (names, name_glob, clients, clients_glob, name_blocks), and push that down to varz_server, or varz_server_storage_manager for resolution. Not necessary though.

There's a bunch of formatting that needs cleaning up before this is merged (80 character lime limit, unused imports, documentation, etc.). If you'd like, I can go through now and annotate it.

@xslogic
Copy link
Author

xslogic commented Aug 22, 2012

Hey Kevin,

I think we will keep the Param class as it is for the time being.. will make it more generic as and when the need arises.
By the way.. glob support is currently only available for the GET (non batch) requests. I didnt see a point in adding it for the batch requests, considering the fact that the batch requests were meant to accept lists of names and the name parameter cannot anyway be specified along with the glob paramater.

I will take care of the formatting and the documentation.

-Arun

* Made some formatting changes (adhere to 80 char limit)
* removed glob parameter from memory_redis_engine
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