[fix][doc] Rewrite the steps of docker-deploy #17853

liangyepianzhou · 2022-09-27T11:44:09Z

Motivation

The old implementation is incomplete.

Modification

Rewrite it, and add a docker-compose.yaml.

preview

### Documentation - [x] `doc`

Document in the google doc: https://docs.google.com/document/d/1MS9KDvmdOBMK58kZtHlqOUl5z7oNBjPNXayP7_uu_D4/edit#heading=h.5mlhk32k72hb

Website local preview:

### Motivation The old implementation is incompletely. ### Modification Rewrite it.

Anonymitaet · 2022-09-28T00:52:25Z

@Demogorgon314
could you please review this PR from a technical perspective? Thank you!

site2/docs/deploy-docker.md

Demogorgon314 · 2022-09-28T01:52:00Z

I also have another idea about docker deployment, we can add a docker-compose to simplify the docker deployment.

site2/docs/deploy-docker.md

liangyepianzhou · 2022-09-30T05:37:27Z

@Demogorgon314 @momo-jun The docker-compose.yaml has been completed at the google doc, you can review it when you have time.

site2/docs/deploy-docker.md

Demogorgon314 · 2022-09-30T13:34:04Z

After trying the docker-compose.yaml . I find out the health check implement is wrong.

For more details, please refer to https://gist.github.com/Demogorgon314/c0d397a21c5a05b40235bb43272a4a88

liangyepianzhou · 2022-09-30T14:50:08Z

After trying the docker-compose.yaml . I find out the health check implement is wrong.

For more details, please refer to https://gist.github.com/Demogorgon314/c0d397a21c5a05b40235bb43272a4a88

I write the health check according to this PR https://github.com/apache/skywalking-rust/pull/9/files#diff-29d8d8c569345f7e4339e3bab5342fa39d73d2293303d372c0f00cd04ad242eb
And the cluster can be started up correctly, right?
There may be some flaws in the health check here, but it does not affect the correct deployment of the pulsar cluster.
And I think the correct way to solve the problem should be to comment on the original google doc instead of copying it elsewhere for modification.

liangyepianzhou · 2022-09-30T15:08:24Z

@Demogorgon314 Thanks for your support of my work. After verifying the changes for the health check and other optimizations, I will accept them in the google doc. And then if you have other suggestions, feel free to leave comments in the google doc.

liangyepianzhou · 2022-09-30T15:56:33Z

There are some exceptions when using the docker-compose.yaml to start up the cluster. The exception happens after 9 minutes, Is this an expected behavior?

Demogorgon314 · 2022-09-30T16:23:44Z

@liangyepianzhou Thanks for reporting the issue. Maybe you should pull the latest version of Pulsar image. See #12353 already removed the check.

liangyepianzhou · 2022-09-30T16:28:37Z

@liangyepianzhou Thanks for reporting the issue. Maybe you should pull the latest version of Pulsar image. See #12353 already removed the check.

The image of the docker is configured in the docker-compose.yaml, It is the latest, right?

Demogorgon314 · 2022-10-01T00:47:31Z

Actually, "latest" is just a label, just like other versions. When you pull the ”latest“ image to local, the image will never update.

You need to delete the ”latest“ image and re-pull it again.

site2/docs/deploy-docker.md

momo-jun

Thanks for refreshing this document. Only a minor suggestion left.

Anonymitaet · 2022-10-11T02:09:30Z

@liangyepianzhou Thanks for your contribution!

In this PR, the correct local preview screenshots are not attached in the PR description.

Can you preview the changes locally and attach the screenshots? So that we can verify everything goes fine.
After that, we can merge the doc PR. Please do not merge a doc PR without preview screenshots in the PR description.

tisonkun · 2022-10-11T04:30:38Z

site2/docs/deploy-docker.md

-## Prepare
-
-To run Pulsar on Docker, you need to create a container for each Pulsar component: ZooKeeper, BookKeeper and broker. You can pull the images of ZooKeeper and BookKeeper separately on [Docker Hub](https://hub.docker.com/), and pull a [Pulsar image](https://hub.docker.com/r/apachepulsar/pulsar-all/tags) for the broker. You can also pull only one [Pulsar image](https://hub.docker.com/r/apachepulsar/pulsar-all/tags) and create three containers with this image. This tutorial takes the second option as an example.
+—


This is an obvious syntax error that fails the site build.

Please be careful when reviewing...Always previewing your changes helps.

I made a suggestion to modify the description template. I think we should guarantee correctness from the process and institutional should be, rather than expecting reviewers and authors always to be correct.

No. The community runs by meritocracy so reviewers should be careful. It's not irreversible to make mistake like this and I'm not blaming you here. But send a notice that you should follow the process. Adding more description hints you to follow the process also but I believe few people really read the whole description.

Shouldn't there be a sanity check CI job for any documentation change in the repo if full pipeline is too expensive?

liangyepianzhou · 2022-10-11T04:32:29Z

@Anonymitaet Thanks for the reminder again.
Here is a very interesting point.
When you told me last time that I needed to attach a screenshot of the preview, I understood the preview from the perspective of a technician, so I attached a screenshot of the successful deployment of the cluster.
But in fact, you meant to attach a preview screenshot of the document/website.
I think this question may also confuse other newcomers.
To solve this problem, I have a small suggestion about the description template.

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->


Document in the google doc: <!-- ENTER URL HERE --> 
<!-- If your PR contains a lot of doc changes -->

Website local preview <!-- ATTACH PREVIEW SCREENSHOTS -->
<!-- If your PR contains doc changes, you need to run `sh start.sh` at `$pwd`/site2/website and attach the preview screenshots here. -->

tisonkun · 2022-10-11T04:36:38Z

@liangyepianzhou I don't think it helps so much. Longer and longer templates attract little careness to read, except we add some clear failure.

Basically, the doc reviewers should be careful. And a clean solution is to sort out the site build process and add a CI task to pre-commit site changes.

liangyepianzhou · 2022-10-11T04:55:06Z

@tisonkun When the PR contains more documents, we hope this document can be placed in Google Docs or other documents first so that it can be reviewed more quickly.
But where to put it is a problem. If we put it in the comment, it will not be easy to find it when there are many comments.
So it would be helpful to have a suitable place to put documentation links in the description.
Both CI tasks and preview screenshots can solve this problem very well, but there must be one. And if use the preview screenshot, then it should be part of the description, not a tacit agreement of people who often do this.

liangyepianzhou · 2022-10-11T04:58:12Z

I don't think it helps so much. Longer and longer templates attract little careness to read, except we add some clear failure.

@tisonkun If this PR is about documentation, he only cares about the description in the document section. If it's not about the documentation, maybe he'll just choose doc-not-needed in the documentation section and don't care about anything else. So I think adding these four lines of description will help a lot for those who need him, who are just starting work for documentation, without burdening others who don't work for documentation.

Anonymitaet · 2022-10-11T08:26:53Z

@Anonymitaet Thanks for the reminder again. Here is a very interesting point. When you told me last time that I needed to attach a screenshot of the preview, I understood the preview from the perspective of a technician, so I attached a screenshot of the successful deployment of the cluster. But in fact, you meant to attach a preview screenshot of the document/website. I think this question may also confuse other newcomers. To solve this problem, I have a small suggestion about the description template.
### Documentation



- [ ] `doc` 
- [ ] `doc-required` 
- [ ] `doc-not-needed` 
- [ ] `doc-complete` 


Document in the google doc:  


Website local preview

Hi @liangyepianzhou thanks for your feedback!

Local preview screenshots

When talking about "local preview screenshots", it refers to the "local preview screenshots" in #14314 (comment) rather than "preview screenshots" in #17853 (comment)

Google docs

Google docs is not a must for every doc PR. But if you working on a large doc set, to accelerate the review process and collaborate efficiently, it is recommended to follow the steps below:

(1) Draft in a google doc
(2) Request technical reviews and incorporate comments
(3) Request technical writing reviews and incorporate comments
(4) Send a PR, attach the google doc link (to show your writings have been reviewed by others) and local preview screenshots (to show everything goes fine)

In this way, you can get PRs merged faster and more smoothly.

cc @DaveDuggins @momo-jun @D-2-Ed

🔹🔹🔹

Besides, I've added a simple reminder to the PR template, #17999, PTAL @liangyepianzhou @tisonkun

Demogorgon314 · 2022-10-11T08:54:41Z

And a clean solution is to sort out the site build process and add a CI task to pre-commit site changes.

@tisonkun Agree, adding a CI task to give a site changes preview is a better solution.

tisonkun · 2022-10-11T09:00:42Z

adding a CI task

We cannot now, although XD.

The build scripts and relative paths are quite brittle to modularize and automatize: apache/pulsar-site#246 (comment)

It still needs some efforts to refactor the build process.

[fxi][doc] Rewrite the steps of docker-deploy

9c4d44c

### Motivation The old implementation is incompletely. ### Modification Rewrite it.

liangyepianzhou requested review from momo-jun, codelipenghui, congbobo184 and gaoran10 September 27, 2022 11:44

github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 27, 2022

Demogorgon314 requested changes Sep 28, 2022

View reviewed changes