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

Add batch query capabilites to elastic io #462

Conversation

chinmay-gupte
Copy link
Contributor

Currently, its possible to only make one single query using the DiscoveryIO interface and specifically by ElasticIO implementation.
This PR adds that capability

TODO

  • More tests

@tilogaat
Copy link
Contributor

tilogaat commented May 6, 2015

@chinmay-gupte Is this going to be wired to the metrics list api call? For example, /metrics/search="one.metric."&search="two.metric." ?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.16%) to 51.58% when pulling 8253fe8 on chinmay-gupte:chinmay/add_batch_query_capabilites_to_elasticIO into 7072b54 on rackerlabs:master.

@chinmay-gupte
Copy link
Contributor Author

@tilogaat Right now the idea is to just use this to start using ES to grab units in mplot handler. But using it for searching metrics in metrics list api is definitely something on the table. Something worth putting out there is once tokenization of metrics lands, metrics list api will already have a richer query interface, making this support for metrics list redundant.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 51.46% when pulling 8c61819 on chinmay-gupte:chinmay/add_batch_query_capabilites_to_elasticIO into 7072b54 on rackerlabs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 51.46% when pulling 8c61819 on chinmay-gupte:chinmay/add_batch_query_capabilites_to_elasticIO into 7072b54 on rackerlabs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 51.46% when pulling 8c61819 on chinmay-gupte:chinmay/add_batch_query_capabilites_to_elasticIO into 7072b54 on rackerlabs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 51.46% when pulling 8c61819 on chinmay-gupte:chinmay/add_batch_query_capabilites_to_elasticIO into 7072b54 on rackerlabs:master.

if (query.indexOf(tenantId.name()) >= 0) {
throw new Exception("Illegal query: " + query);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

do we not need this anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tilogaat I am not sure what this line of code was doing. The original intent seemed to not allow a query which is cross-tenant, like tenant A cannot search for metrics belonging to tenant B. But all it was doing was searching to see if the string tenantId does not exist in the query.
For what its worth, I checked whether cross-tenant discovery is possible, and it seems like its not. We index tenantId along with metricName and unit as terms, and while searching we pass the tenantId from the uri and use it as a must contain match, so its impossible to search for cross-tenant metrics.

@tilogaat
Copy link
Contributor

tilogaat commented May 7, 2015

Thanks @chinmay-gupte! +1

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 51.48% when pulling c8a22b9 on chinmay-gupte:chinmay/add_batch_query_capabilites_to_elasticIO into 7072b54 on rackerlabs:master.

chinmay-gupte added a commit that referenced this pull request May 7, 2015
…pabilites_to_elasticIO

Add batch query capabilites to elastic io
@chinmay-gupte chinmay-gupte merged commit 2e82ab2 into rax-maas:master May 7, 2015
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.

None yet

3 participants