Skip to content

Scatter-gather support for Query API #38

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

Merged
merged 53 commits into from
Sep 17, 2019
Merged

Scatter-gather support for Query API #38

merged 53 commits into from
Sep 17, 2019

Conversation

jeqo
Copy link
Collaborator

@jeqo jeqo commented Sep 4, 2019

Work to be done in order to support scaling storage.

Currently only one instance is supported to get complete response from data stored. If scaled, responses will get partitioned.

Using one instance is limiting storage usage, as when load is high, it starts falling back as one instance cannot keep pace with the ingestion load.

Initial design

In order to query all instances, we need to have a channel where stores can communicate:

------client requests----> | zipkin server 0  |<--+ store requests
                                                  |
------client requests----> | zipkin server 1  |<--+ store requests
                                                  |
------client requests----> | zipkin server 2  |<--+ store requests

Then:

  • when receiving client requests: storage will make store requests to other instances.
  • when receiving store requests: storage will make requests to local state.

We can expose store endpoint in another port (e.g. 9412), including metadata and metrics that allow better debugging.

Fix #36

@jeqo
Copy link
Collaborator Author

jeqo commented Sep 4, 2019

@adriancole this might be a long one :)

Copy link
Collaborator Author

@jeqo jeqo left a comment

Choose a reason for hiding this comment

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

Currently working when testing one instance. Requires fixing tests and validate approach on scaling instances.

@jeqo jeqo marked this pull request as ready for review September 9, 2019 22:08
@jeqo
Copy link
Collaborator Author

jeqo commented Sep 12, 2019

thanks @adriancole and @anuraaga for your feedback! Hope it looks much better now :)

about reusing zipkin-server: is it something we would be able to do right now or requires changes on armeria first?

@anuraaga
Copy link
Contributor

I think until armeria supports Consumer, we can go ahead and add a provided dependency on armeria-spring and use the custom type so this PR works with the current artifacts.

@jeqo
Copy link
Collaborator Author

jeqo commented Sep 14, 2019

@anuraaga I'd like to keep the current impl with port 9412, and fix integration and reuse zipkin server on a new PR. I've tried to use armeria-spring but haven't manage to mount the http service.

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Sep 16, 2019 via email

trustin pushed a commit to line/armeria that referenced this pull request Sep 16, 2019
…2070)

I think it's useful to be able to customize with spring without using custom types, for example to allow creating a plugin module that just configures a server without needing to worry about armeria-spring. It's sort of similar to how Spring supports both custom annotations and `javax.inject` annotaions. 

Idea came up in openzipkin-contrib/zipkin-storage-kafka#38
@anuraaga
Copy link
Contributor

Sure no problem 👍

@jeqo
Copy link
Collaborator Author

jeqo commented Sep 17, 2019

great! just created #40 to follow up the discussion.

is there any other feedback before merging this?

@@ -25,8 +25,10 @@ services:
zipkin:
image: jeqo/zipkin-kafka
container_name: zipkin
hostname: zipkin
Copy link
Contributor

Choose a reason for hiding this comment

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

anywhere setting hostname explicitly, comment why important. If important we should probably consider changing upstream docker-compose also..

Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

Please do merge this. If you don't mind, I'd like to do a follow-up commit to use routine build infra.

@jeqo jeqo merged commit e8d0fef into master Sep 17, 2019
@codefromthecrypt codefromthecrypt deleted the scatter branch September 17, 2019 00:59
eugene70 pushed a commit to eugene70/armeria that referenced this pull request Oct 16, 2019
…ine#2070)

I think it's useful to be able to customize with spring without using custom types, for example to allow creating a plugin module that just configures a server without needing to worry about armeria-spring. It's sort of similar to how Spring supports both custom annotations and `javax.inject` annotaions. 

Idea came up in openzipkin-contrib/zipkin-storage-kafka#38
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
…ine#2070)

I think it's useful to be able to customize with spring without using custom types, for example to allow creating a plugin module that just configures a server without needing to worry about armeria-spring. It's sort of similar to how Spring supports both custom annotations and `javax.inject` annotaions. 

Idea came up in openzipkin-contrib/zipkin-storage-kafka#38
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.

Scatter-gather support on Query APIs
3 participants