Skip to content

Fix typo in functions-work.yaml, add functionWebServiceUrl in yaml#13345

Closed
hezhangjian wants to merge 1 commit intoapache:masterfrom
hezhangjian:fix-typo-functions-worker
Closed

Fix typo in functions-work.yaml, add functionWebServiceUrl in yaml#13345
hezhangjian wants to merge 1 commit intoapache:masterfrom
hezhangjian:fix-typo-functions-worker

Conversation

@hezhangjian
Copy link
Member

Modifications

Fix typo in functions-work.yaml, add functionWebServiceUrl in yaml

Documentation

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

Need to update docs?

  • no-need-doc

trival changes, no need doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 15, 2021
@hezhangjian
Copy link
Member Author

@merlimat merlimat added this to the 2.10.0 milestone Dec 16, 2021
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

@shoothzj - can you provide a little more context for this change? Explicitly adding a default to the functions_worker.yml will change the default behavior. Thanks.

# For TLS:
# webServiceUrl=https://localhost:8443/
pulsarWebServiceUrl: http://localhost:8080
functionWebServiceUrl: http://localhost:8080
Copy link
Member

Choose a reason for hiding this comment

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

I think this will change the behavior of the following code if workerHostname is overridden in this file and the operator is relying on the following conditional to set the functionWebServiceUrl. Why do we need to add this to the configuration?

final String functionWebServiceUrl = StringUtils.isNotBlank(workerConfig.getFunctionWebServiceUrl())
? workerConfig.getFunctionWebServiceUrl()
: (workerConfig.getTlsEnabled()
? workerConfig.getWorkerWebAddressTls() : workerConfig.getWorkerWebAddress());

public String getWorkerWebAddress() {
return String.format("http://%s:%d", this.getWorkerHostname(), this.getWorkerPort());
}

public String getWorkerHostname() {
if (isBlank(this.workerHostname)) {
this.workerHostname = unsafeLocalhostResolve();
}
return this.workerHostname;
}

@hezhangjian
Copy link
Member Author

@shoothzj - can you provide a little more context for this change? Explicitly adding a default to the functions_worker.yml will change the default behavior. Thanks.

@michaeljmarshall
the field functionWebServiceUrl is widely used when people deploy function-works and pulsar-broker divided.
It is more reasonable for this frequently used configuration parameter to appear in the configuration file.
cc @merlimat @315157973

@hezhangjian
Copy link
Member Author

ping @michaeljmarshall

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

adding functionWebServiceUrl is a breaking change, please make that separately from other changes and bring it up to discussion on the mailing list for decision making.

@hezhangjian
Copy link
Member Author

@lhotari Ok, I will start a thread on mail list, for now I will close this PR :)

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

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants