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

Update documentation and remove unesessary configuration #3591

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

mrproliu
Copy link
Contributor

Features of Zipkin v3 have been almost developed, so this pull request is intended for updating the documentation and removing unnecessary configuration items.

@mrproliu mrproliu added this to the 3.0.0 milestone Nov 14, 2023
@mrproliu mrproliu requested a review from wu-sheng November 14, 2023 09:33
zipkin-server/README.md Outdated Show resolved Hide resolved

## Configuration
We support ENV variable configuration, such as `STORAGE_TYPE=cassandra3`, as they are familiar to
We support ENV variable configuration, such as `ZIPKIN_STORAGE=cassandra`, as they are familiar to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
We support ENV variable configuration, such as `ZIPKIN_STORAGE=cassandra`, as they are familiar to
We support ENV variable configuration, such as `ZIPKIN_STORAGE=cassandra3`, as they are familiar to

I think we consistent with previous. Could you rechecck?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update the names to cassandra3

zipkin-server/README.md Outdated Show resolved Hide resolved
Comment on lines 445 to 446
* `ZIPKIN_STORAGE_BANYANDB_HOST`: The host of BanyanDB. Defaults to `127.0.0.1`.
* `ZIPKIN_STORAGE_BANYANDB_PORT`: The port of BanyanDB. Defaults to `17912`.
Copy link
Member

Choose a reason for hiding this comment

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

These should be merged as targets. Could you recheck your dependency version of OAP and sync the doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The configuration and documentation have been updated.

Comment on lines 464 to 468
### Legacy (v1) storage components
The following components are no longer encouraged, but exist to help aid
transition to supported ones. These are indicated as "v1" as they use
data layouts based on Zipkin's V1 Thrift model, as opposed to the
simpler v2 data model currently used.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this statement? I think the new of v3 includes making MySQL and PG back to be available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Been deleted.

zipkin-server/README.md Outdated Show resolved Hide resolved
@wu-sheng wu-sheng merged commit 062d493 into openzipkin:zipkin-v3 Nov 15, 2023
8 checks passed
@mrproliu mrproliu deleted the documentation branch November 15, 2023 03:53
@@ -92,14 +92,13 @@ accordingly.

Ex. If your link name is "storage" for an Elasticsearch container:
```
ES_HOSTS=http://$STORAGE_PORT_9200_TCP_ADDR:9200
ZIPKIN_STORAGE_ES_CLUSTER_NODES=$STORAGE_PORT_9200_TCP_ADDR:9200
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we maintain both variables and deprecate the former? As far as I understand we want to keep retrocompatibility for ease migration.

Copy link
Member

Choose a reason for hiding this comment

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

We could keep some consistent with before. But things like storage selector, it is very skywalking style, can't be aligned with true/false style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants