Skip to content

[improve][doc] Add chroot path related informations - #18040

Merged
tisonkun merged 4 commits into
apache:masterfrom
yoda-mon:update-baremetal-cluster-doc
Nov 14, 2022
Merged

[improve][doc] Add chroot path related informations#18040
tisonkun merged 4 commits into
apache:masterfrom
yoda-mon:update-baremetal-cluster-doc

Conversation

@yoda-mon

@yoda-mon yoda-mon commented Oct 13, 2022

Copy link
Copy Markdown
Contributor

Motivation

On this PR #13985, /my-chroot-path was added to the URLs.
If users follow the document, bookkeeper.conf and broker.conf lacks the informations about the path and fail to set up the cluster.

Modifications

Add chroot path related informations to the documents and add zk:// prefix to metadata URLs.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Screen Shot 2022-10-13 at 19 49 19

Screen Shot 2022-10-13 at 19 49 59

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions Bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Oct 13, 2022
@Anonymitaet

Copy link
Copy Markdown
Member

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

@Anonymitaet

Copy link
Copy Markdown
Member

ping @mattisonchao

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd prefer we don't use chroot at all since it's an advanced topic and should not be the default information delivered to users.

cc @RobertIndie it's related to your previous patch #13985. Please take a look.

@yoda-mon

Copy link
Copy Markdown
Contributor Author

Thank you for your comment.
@RobertIndie chroot is the point that I struggled at first as a Pulsar beginner. If it is advanced topic I would emit the usage of chroot from the doc.

@RobertIndie

Copy link
Copy Markdown
Member

I'd prefer we don't use chroot at all since it's an advanced topic and should not be the default information delivered to users.

@tisonkun @yoda-mon +1. It's an advanced topic. We can use a separate topic to talk about it.

@tisonkun

Copy link
Copy Markdown
Member

@RobertIndie Then let's remove it first. I think we cannot cover every ZK advanced topics in the Pulsar doc site. But if chroot a significant topic, we may find a place to discuss it. You may contact with @momo-jun @Anonymitaet for where in the information architecture is proper to handle such content.

@yoda-mon for this PR, let's revert the chroot command from #13985. You may check the corresponding files under site2/website/versioned_docs/version-2.10.x where we're actively maintaining and you should update it simultaneously.

@yoda-mon

Copy link
Copy Markdown
Contributor Author

@tisonkun Thank you for your advice, I followed.
And let me confirm one thing, I added zk:// prefix to the example because the broker.conf's comment shows this style first and initialize-cluster-metadata's example shows.
I recognized that both styles, the prefix exists or not, work fine. If it is preferable to remove the prefix, I will revert the commit.

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

cc @RobertIndie @eolivelli please give it a look if you have spare time :)

@tisonkun
tisonkun merged commit f548d1b into apache:master Nov 14, 2022
@tisonkun tisonkun changed the title [improve][docs] Add chroot path related informations [improve][doc] Add chroot path related informations Nov 14, 2022
@tisonkun

Copy link
Copy Markdown
Member

@yoda-mon Thanks for your contribution!

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

Labels

doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants