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.

Sign up for GitHub

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

Jump to bottom

Update deploy-bare-metal.md #12432

Merged
merged 3 commits into from Nov 18, 2021
Merged

Update deploy-bare-metal.md #12432

merged 3 commits into from Nov 18, 2021

Conversation

diegobelotti
Copy link
Contributor

@diegobelotti diegobelotti commented Oct 20, 2021 •
edited

no details for NAT. It is useful for distributed cluster.

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@diegobelotti
Update deploy-bare-metal.md
6c27f8c 
no details for NAT. It is useful for distributed cluster.
@eolivelli
Copy link
Contributor

@diegobelotti:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@eolivelli
Copy link
Contributor

@diegobelotti:Thanks for providing doc info!

@eolivelli eolivelli added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Oct 20, 2021
@Anonymitaet
Copy link
Member

@Demogorgon314 can you help review this PR from a technical perspective? Thanks

Demogorgon314
Demogorgon314 reviewed Oct 21, 2021
View reviewed changes
site2/docs/deploy-bare-metal.md Outdated
> If you only have one machine on which to deploy Pulsar, you only need to add one server entry in the configuration file.

> If your machines are behind NAT use 0.0.0.0 as server entry for the local address.
```example for configuration on server #3
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
```example for configuration on server #3
example for configuration on server.3
```properties

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, this is better.

Demogorgon314
Demogorgon314 reviewed Oct 21, 2021
View reviewed changes
Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

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

Overall LGTM just left a comment. PTAL.

@diegobelotti
Copy link
Contributor Author

@Demogorgon314 can you help review this PR from a technical perspective? Thanks

If the node use external IP in configuration for itself, behind NAT, zookeper service won't start because it tries to put a listener on an external ip that the linux bow doesn't own. Using 0.0.0.0 start a listener on ALL that NAT network traffic can reach.

Demogorgon314
Demogorgon314 approved these changes Oct 22, 2021
View reviewed changes
Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

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

LGTM

@Anonymitaet
Copy link
Member

@diegobelotti any progress? Thanks

@diegobelotti @Anonymitaet
Update site2/docs/deploy-bare-metal.md
1b40c74 
Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
@diegobelotti
Copy link
Contributor Author

@diegobelotti any progress? Thanks

Sorry I'm a newbie on github. There is anything else I should do?

@Anonymitaet
Copy link
Member

@diegobelotti can you incorporate our comments? Thanks

@diegobelotti
Copy link
Contributor Author

@diegobelotti can you incorporate our comments? Thanks

Done. Is this what you mean?

@Anonymitaet Anonymitaet added this to the 2.10.0 milestone Oct 27, 2021
@Anonymitaet
Copy link
Member

@diegobelotti exactly. We can merge this PR after it passes tests.

@Anonymitaet Anonymitaet merged commit 5c12be7 into apache:master Nov 18, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Nov 18, 2021
Merge branch 'up/master' into website/update-2.7.2-chapter
5d131bd 
* up/master:
  Update deploy-bare-metal.md (apache#12432)
  [Broker] Fix producer getting incorrectly removed from topic's producers map (apache#12846)
  Add error log when new jetty client (apache#12840)
  JavaInstanceTest should be AssertEquals (apache#12836)
  [Transaction] Fix transaction flaky test testMaxReadPositionForNormalPublish (apache#12681)
  The problem of two exception handling (apache#12744)
  Fix TopicPoliciesCacheNotInitException issue. (apache#12773)
  Added local filesystem backend for package manager (apache#12708)
  [Java Client] Make userProvidedProducerName final (apache#12849)
  optimize indention in ServerCnx#handleProducer (apache#12854)
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Nov 18, 2021
Merge branch 'up/master' into website/branch-2.6.2-part-1
4f01b73 
* up/master:
  [Issue 12757][broker] add broker config isAllowAutoUpdateSchema (apache#12786)
  Update deploy-bare-metal.md (apache#12432)
  [Broker] Fix producer getting incorrectly removed from topic's producers map (apache#12846)
  Add error log when new jetty client (apache#12840)
  JavaInstanceTest should be AssertEquals (apache#12836)
  [Transaction] Fix transaction flaky test testMaxReadPositionForNormalPublish (apache#12681)
  The problem of two exception handling (apache#12744)
  Fix TopicPoliciesCacheNotInitException issue. (apache#12773)
  Added local filesystem backend for package manager (apache#12708)
dlg99 pushed a commit to dlg99/pulsar that referenced this pull request Nov 23, 2021
@diegobelotti @Anonymitaet @dlg99
Update deploy-bare-metal.md (apache#12432)
dc3717e 
* Update deploy-bare-metal.md

no details for NAT. It is useful for distributed cluster.

* Update site2/docs/deploy-bare-metal.md

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>

* Update deploy-bare-metal.md

AS requested

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
@diegobelotti @Anonymitaet @eolivelli
Update deploy-bare-metal.md (apache#12432)
918577b 
* Update deploy-bare-metal.md

no details for NAT. It is useful for distributed cluster.

* Update site2/docs/deploy-bare-metal.md

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>

* Update deploy-bare-metal.md

AS requested

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
@diegobelotti @Anonymitaet
Update deploy-bare-metal.md (apache#12432)
66196a8 
* Update deploy-bare-metal.md

no details for NAT. It is useful for distributed cluster.

* Update site2/docs/deploy-bare-metal.md

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>

* Update deploy-bare-metal.md

AS requested

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes

@Anonymitaet Anonymitaet Anonymitaet approved these changes

Assignees

@diegobelotti diegobelotti

Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@diegobelotti @eolivelli @Anonymitaet @Demogorgon314