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

Updated docs/deployment.md with reordering subtopics #475

Closed
wants to merge 1 commit into from

Conversation

ideepika
Copy link
Contributor

No description provided.

@ideepika ideepika changed the title Issue/#472 Added Docker run command for Cassandra Oct 16, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ed84662 on dexter816:Issue/#472 into 9523cbe on jaegertracing:master.

@jpkrohling
Copy link
Contributor

Thanks for this contribution, @dexter816 ! I added a few comments to the review but it looks good in general!

I'm curious, though, as to what's different on this PR compared to #473 . If this is the same task, then you could add more commits to that PR. If you are unsure on how to update a PR, let me know and I can guide you.

I would also ask you to squash the commits from this PR, as there seems to be a merge commit on it. You can squash the commits using git rebase -i master, leaving the first as pick (or r to change the commit message) and the remaining ones as squash.

```
or if you don't have the source code
```
docker run -e MODE=... jaegertracing/jaeger-cassandra-schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you replace by the actual command? I think I typed this from memory and I might be wrong :)


### Discovery System Integration
```sh
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks odd. Perhaps this needs to be removed?

image

across several collectors ([issue 213](https://github.com/uber/jaeger/issues/213)).
#### Shards and Replicas for ElasticSearch indices

=======
Copy link
Contributor

Choose a reason for hiding this comment

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

This also looks odd:

image

Some editors, such as Visual Studio Code, have a markdown viewer that might be useful:
image


Example:
```
docker run -it --rm -p14267:14267 -p14268:14268
Copy link
Contributor

Choose a reason for hiding this comment

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

This should either be all in one line, or there should be a \ at the end of the line, so that readers can just copy/paste from the instructions to the terminal.


## Query Service & UI

**jaeger-query** serves the API endpoints and a React/Javascript UI.
The service is stateless and is typically run behind a load balancer, e.g. nginx.

An example to test Query Service:
```
docker run -it -p16686:16686 jaegertracing/jaeger-query:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: either add all in one line, or add \ at the end of the line.

@ideepika
Copy link
Contributor Author

@jpkrohling thanks for the review ,yeah I was unsure how to update pull request ,I made the amendments you suggested , looking at updating PR , thanks again :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 18fb975 on dexter816:Issue/#472 into 9523cbe on jaegertracing:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 15883de on dexter816:Issue/#472 into 0302c8d on jaegertracing:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4291760 on dexter816:Issue/#472 into 11d7d47 on jaegertracing:master.

@ideepika ideepika changed the title Added Docker run command for Cassandra Updated docs/deployment.md with reordering subtopics Oct 21, 2017
In the future we will support different service discovery systems to
dynamically load balance
across several collectors ([issue
213](https://github.com/uber/jaeger/issues/213)).
Copy link
Member

Choose a reason for hiding this comment

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

what is the purpose of these formatting changes? they seem to make this diff twice as big as it needs to be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed that ,thanks :)

Changes made:

-replaced ElasticSearch to Elasticsearch
-updated example commands for better readibilty
-minor updates in docs/deployment.md

Closes: #472
Signed-off-by: Deepika Upadhyay deepikaupadhyay01@gmail.com
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9c0d6c9 on dexter816:Issue/#472 into 11d7d47 on jaegertracing:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9c0d6c9 on dexter816:Issue/#472 into 11d7d47 on jaegertracing:master.

@pavolloffay
Copy link
Member

Closing docs have been moved to https://github.com/jaegertracing/documentation repo

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.

5 participants