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

Fix the cluster setup instructions. #21

Merged
merged 3 commits into from
Sep 20, 2016
Merged

Fix the cluster setup instructions. #21

merged 3 commits into from
Sep 20, 2016

Conversation

radekg
Copy link
Contributor

@radekg radekg commented Sep 18, 2016

Motivation

The current cluster setup instructions are inaccurate.

Modifications

Updated cluster setup instructions.

Result

Reduce the time it takes to setup a cluster by removing the time it takes to find all this info yourself.

@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@yahoocla
Copy link

CLA is valid!

server.1=zk1.us-west.example.com:2181
server.2=zk2.us-west.example.com:2181
server.3=zk3.us-west.example.com:2181
server.1=zk1.us-west.example.com:2888:3888
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 default quorum and leader election ports are 2888:3888 for port 2181.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point.

--global-zookeeper zk1.us-west.example.com:2184 \
--service-url http://pulsar.us-west.example.com:8080/ \
--service-url-tls https://pulsar.us-west.example.com:8443/
$ bin/pulsar initialize-cluster-metadata --cluster us-west \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing command.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch

@@ -183,6 +183,9 @@ ledgerDirectories=data/bookkeeper/ledgers

# Point to local ZK quorum
zkServers=zk1.example.com:2181,zk2.example.com:2181,zk3.example.com:2181

# Change the ledger manager type
ledgerManagerType=hierarchical
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 broker expects hierarchical while default is flat. The omission causes the failure while starting the broker. The failure goes along the lines of:

2016-09-18 20:31:08,601 - ERROR [main:PulsarService@311] - Configured layout org.apache.bookkeeper.meta.HierarchicalLedgerManagerFactory does not match existing layout org.apache.bookkeeper.meta.FlatLedgerManagerFactory
java.io.IOException: Configured layout org.apache.bookkeeper.meta.HierarchicalLedgerManagerFactory does not match existing layout org.apache.bookkeeper.meta.FlatLedgerManagerFactory
    at org.apache.bookkeeper.meta.LedgerManagerFactory.newLedgerManagerFactory(LedgerManagerFactory.java:159)
    at org.apache.bookkeeper.client.BookKeeper.<init>(BookKeeper.java:299)
    at org.apache.bookkeeper.client.BookKeeper.<init>(BookKeeper.java:266)
    at org.apache.bookkeeper.client.BookKeeper.<init>(BookKeeper.java:243)
    at com.yahoo.pulsar.broker.BookKeeperClientFactoryImpl.create(BookKeeperClientFactoryImpl.java:79)
    at com.yahoo.pulsar.broker.ManagedLedgerClientFactory.<init>(ManagedLedgerClientFactory.java:38)
    at com.yahoo.pulsar.broker.PulsarService.start(PulsarService.java:231)
    at com.yahoo.pulsar.PulsarBrokerStarter.main(PulsarBrokerStarter.java:59)
2016-09-18 20:31:08,603 - ERROR [main:PulsarBrokerStarter@62] - Failed to start pulsar service.
com.yahoo.pulsar.broker.PulsarServerException: java.io.IOException: Configured layout org.apache.bookkeeper.meta.HierarchicalLedgerManagerFactory does not match existing layout org.apache.bookkeeper.meta.FlatLedgerManagerFactory
    at com.yahoo.pulsar.broker.PulsarService.start(PulsarService.java:312)
    at com.yahoo.pulsar.PulsarBrokerStarter.main(PulsarBrokerStarter.java:59)
Caused by: java.io.IOException: Configured layout org.apache.bookkeeper.meta.HierarchicalLedgerManagerFactory does not match existing layout org.apache.bookkeeper.meta.FlatLedgerManagerFactory
    at org.apache.bookkeeper.meta.LedgerManagerFactory.newLedgerManagerFactory(LedgerManagerFactory.java:159)
    at org.apache.bookkeeper.client.BookKeeper.<init>(BookKeeper.java:299)
    at org.apache.bookkeeper.client.BookKeeper.<init>(BookKeeper.java:266)
    at org.apache.bookkeeper.client.BookKeeper.<init>(BookKeeper.java:243)
    at com.yahoo.pulsar.broker.BookKeeperClientFactoryImpl.create(BookKeeperClientFactoryImpl.java:79)
    at com.yahoo.pulsar.broker.ManagedLedgerClientFactory.<init>(ManagedLedgerClientFactory.java:38)
    at com.yahoo.pulsar.broker.PulsarService.start(PulsarService.java:231)
    ... 1 more

Copy link
Contributor

@merlimat merlimat Sep 19, 2016

Choose a reason for hiding this comment

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

I think the misunderstanding here is that these instructions are supposed to point to the config file keys that needs to be changed. The text in the block is not a valid complete configuration.

If you look in the conf/bookkeeper.conf, the ledgerManagerType=hierarchical line is already there.

Perhaps the text should make more clear to use the conf/bookkeeper.conf as the starting point and edit the specified lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might be the case, indeed. However, these changes are absolute minimum to make it work without having to modify existing files (i.e. create bare minimum viable config). When using configuration management in any form, it might be easier to just apply the minimum config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Main issue is that the minimum viable config might not be the best config for production use. In the broker config we can fix that, and already the conf file should have the same values that are taken as default. In BookKeeper config, however, we want to have different values from the BK defaults.

@@ -221,6 +224,7 @@ zookeeperServers=zk1.us-west.example.com:2181,zk2.us-west.example.com:2181,zk3.u
globalZookeeperServers=zk1.us-west.example.com:2184,zk2.us-west.example.com:2184,zk3.us-west.example.com:2184

clusterName=us-west
statusFilePath=broker-status-file
Copy link
Contributor Author

@radekg radekg Sep 18, 2016

Choose a reason for hiding this comment

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

This setting is required. The broker will fail to start with a cryptic error. The error message could be improved, it does not mention that the required setting is missing in configuration. It only says something along the lines of required statusFilePath not set without pointing to configuration.

Copy link
Contributor Author

@radekg radekg Sep 18, 2016

Choose a reason for hiding this comment

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

Maybe worth changing the default here? https://github.com/yahoo/pulsar/blob/master/conf/broker.conf#L78
The default is a little bit odd: statusFilePath=/usr/local/apache/htdocs

Copy link
Contributor

Choose a reason for hiding this comment

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

As per below comment: the example only states the configuration values to modify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This setting did not work for me, is there a default provided in the code? Or does this setting rely on being set in the file? If the latter, might be worth providing a sane default in the code so the broker does not error if the user misses this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd swear I setup the test env with the above step and this setting was not preventing the broker from functioning. I need some time to look into it again

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tried again to double-check. The status file settings that are in the conf/broker.conf file are "fine", in the sense that they don't prevent the broker from starting correctly.

About the statusFilePath setting, this is used to control whether the broker should be considered as "in-rotation" or "out-of-rotation" from an external monitor process. I'm not sure how useful would this be outside of Yahoo though.

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 just remove this statusFilePath addition? I have that addressed in #28

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@radekg
Copy link
Contributor Author

radekg commented Sep 18, 2016

Figuring out all these has been documented here: #19.

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time for this. Just a couple of comment on the required conf fields

server.1=zk1.us-west.example.com:2181
server.2=zk2.us-west.example.com:2181
server.3=zk3.us-west.example.com:2181
server.1=zk1.us-west.example.com:2888:3888
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point.

--global-zookeeper zk1.us-west.example.com:2184 \
--service-url http://pulsar.us-west.example.com:8080/ \
--service-url-tls https://pulsar.us-west.example.com:8443/
$ bin/pulsar initialize-cluster-metadata --cluster us-west \
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch

--zookeeper zk1.us-west.example.com:2181 \
--global-zookeeper zk1.us-west.example.com:2184 \
--service-url http://pulsar.us-west.example.com:8080/ \
--service-url-tls https://pulsar.us-west.example.com:8443/
Copy link
Contributor

Choose a reason for hiding this comment

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

Last line seems to have extra spaces compared to the rest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected in a6df343

@@ -183,6 +183,9 @@ ledgerDirectories=data/bookkeeper/ledgers

# Point to local ZK quorum
zkServers=zk1.example.com:2181,zk2.example.com:2181,zk3.example.com:2181

# Change the ledger manager type
ledgerManagerType=hierarchical
Copy link
Contributor

@merlimat merlimat Sep 19, 2016

Choose a reason for hiding this comment

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

I think the misunderstanding here is that these instructions are supposed to point to the config file keys that needs to be changed. The text in the block is not a valid complete configuration.

If you look in the conf/bookkeeper.conf, the ledgerManagerType=hierarchical line is already there.

Perhaps the text should make more clear to use the conf/bookkeeper.conf as the starting point and edit the specified lines.

@@ -221,6 +224,7 @@ zookeeperServers=zk1.us-west.example.com:2181,zk2.us-west.example.com:2181,zk3.u
globalZookeeperServers=zk1.us-west.example.com:2184,zk2.us-west.example.com:2184,zk3.us-west.example.com:2184

clusterName=us-west
statusFilePath=broker-status-file
Copy link
Contributor

Choose a reason for hiding this comment

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

As per below comment: the example only states the configuration values to modify

@merlimat merlimat added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 19, 2016
@merlimat merlimat added this to the 1.15 milestone Sep 19, 2016
@merlimat merlimat self-assigned this Sep 19, 2016
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

Looks good!

@merlimat merlimat merged commit b5d0ebb into apache:master Sep 20, 2016
@radekg radekg deleted the update-cluster-setup-docs branch September 20, 2016 22:11
sijie added a commit to sijie/pulsar that referenced this pull request Mar 4, 2018
* Relocate instance and spawner to be under `runtime` package
* fix the pom files and travis build
hrsakai pushed a commit to hrsakai/pulsar that referenced this pull request Dec 10, 2020
* Add golangci-lint for formatting code
hangc0276 referenced this pull request in hangc0276/pulsar May 26, 2021
*Motivation*

Need to port Kafka's coordinator algorithm. It requires using Kafka's timer and deplayed operations.
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.

3 participants