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

get function cluster from broker config when start function worker with broker #10552

Merged
merged 2 commits into from
May 18, 2021
Merged

get function cluster from broker config when start function worker with broker #10552

merged 2 commits into from
May 18, 2021

Conversation

wangjialing218
Copy link
Contributor

Motivation

When start function worker with broker, we need to set pulsarFunctionsCluster in functions_worker.yml, other wise it will fail to start. #2328
If we run broker in k8s, we should set correct config map to change the default value in functions_worker.yml.
In this mode, pulsarFunctionsCluster should always be same with the cluster name in broker.conf, so we can get the setting directly from broker.conf.

Modifications

get function cluster from broker config when start function worker with broker

@wangjialing218 wangjialing218 force-pushed the branch_function_worker_cluster_name branch from ac851c7 to 5aeadd7 Compare May 12, 2021 08:27
eolivelli
eolivelli requested changes May 12, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

we also have to add some unit tests about this change

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
@@ -1515,6 +1515,7 @@ public static WorkerConfig initializeWorkerConfigFromBrokerConfig(ServiceConfigu
String hostname = ServiceConfigurationUtils.getDefaultOrConfiguredAddress(
ServiceConfigurationUtils.getAppliedAdvertisedAddress(brokerConfig));
workerConfig.setWorkerHostname(hostname);
workerConfig.setPulsarFunctionsCluster(brokerConfig.getClusterName());
Copy link
Contributor

Choose a reason for hiding this comment

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

what about setting this brokerConfig.getClusterName() value only if the user did not configure another value ?
otherwise this change may introduce a behaviour change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If user configure another value different with brokerConfig.getClusterName() , broker will fail to start, refer to #2328.
To avoid this to happen, we could force the value same with brokerConfig.getClusterName().

gaoran10
gaoran10 reviewed May 12, 2021
View reviewed changes
Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli
Copy link
Contributor

if setPulsarFunctionsCluster is required for making the broker work.
do we have test cases that set that value ?
we have to fix all of those tests and remove the call to setPulsarFunctionsCluster
does it make sense to you ?

it looks like it is useless to set a value explicitly if you cannot change it

@wangjialing218
Copy link
Contributor Author

if setPulsarFunctionsCluster is required for making the broker work.
do we have test cases that set that value ?
we have to fix all of those tests and remove the call to setPulsarFunctionsCluster
does it make sense to you ?

@eolivelli I have modify exist test case to meet this change, please have a check

it looks like it is useless to set a value explicitly if you cannot change it

Currently default value of FunctionsCluster is loaded from functions_worker.yml.
When we start function woker separately, we need to set FunctionsCluster in functions_worker.yml because we can not get cluster name directly from broker.
And when we start function worker with broker, we can get cluster name directly from broker config, no need to load the value from ‘functions_worker.yml’.

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie sijie added this to the 2.8.0 milestone May 15, 2021
@sijie
Copy link
Member

sijie commented May 15, 2021

@eolivelli please review this.

eolivelli
eolivelli approved these changes May 15, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@wangjialing218 wangjialing218 force-pushed the branch_function_worker_cluster_name branch from 997f0d3 to 06fcb16 Compare May 18, 2021 06:37
@codelipenghui codelipenghui merged commit 3bdc776 into apache:master May 18, 2021
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
@wangjialing218 @yangl
Get function cluster from broker config when start function worker wi…
2c6178a 
…th broker (apache#10552)

### Motivation
When start function worker with broker, we need to set pulsarFunctionsCluster in functions_worker.yml, other wise it will fail to start.   apache#2328
If we run broker in k8s, we should set correct config map to change the default value in functions_worker.yml.
In this mode, pulsarFunctionsCluster should always be same with the cluster name in broker.conf, so we can get the setting directly from broker.conf.

### Modifications

get function cluster from broker config when start function worker with broker
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@wangjialing218 @ciaocloud
Get function cluster from broker config when start function worker wi…
7c602e2 
…th broker (apache#10552)

### Motivation
When start function worker with broker, we need to set pulsarFunctionsCluster in functions_worker.yml, other wise it will fail to start.   apache#2328
If we run broker in k8s, we should set correct config map to change the default value in functions_worker.yml.
In this mode, pulsarFunctionsCluster should always be same with the cluster name in broker.conf, so we can get the setting directly from broker.conf.

### Modifications

get function cluster from broker config when start function worker with broker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaoran10 gaoran10 gaoran10 left review comments

@sijie sijie sijie approved these changes

@eolivelli eolivelli eolivelli approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

Assignees

@wangjialing218 wangjialing218

Labels
None yet
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wangjialing218 @eolivelli @sijie @codelipenghui @gaoran10