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][broker] Fix returns wrong webServiceUrl when both webServicePort and webServicePortTls are set #21633

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Nov 28, 2023

Motivation

If webServicePort and webServicePortTls are set at the same time and create an admin using webServiceAddress to access the topic, you will be stuck in an infinite redirect, the root cause is that the webServiceUrl always returns an https url even if the request is HTTP.

org.apache.pulsar.client.admin.PulsarAdminException: java.util.concurrent.CompletionException: org.apache.pulsar.client.admin.internal.http.AsyncHttpConnector$RetryException: Could not complete the operation. Number of retries has been exhausted. Failed reason: Remotely closed
	at org.apache.pulsar.client.admin.PulsarAdminException.wrap(PulsarAdminException.java:252)
	at org.apache.pulsar.client.admin.internal.BaseResource.sync(BaseResource.java:352)
	at org.apache.pulsar.client.admin.internal.TopicsImpl.createNonPartitionedTopic(TopicsImpl.java:309)
	at org.apache.pulsar.client.admin.Topics.createNonPartitionedTopic(Topics.java:539)
	at org.apache.pulsar.broker.loadbalance.SimpleLoadManagerImplTest.testOwner(SimpleLoadManagerImplTest.java:509)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:139)
	at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:677)
	at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:221)
	at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
	at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:969)
	at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:194)
	at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:148)
	at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at org.testng.TestRunner.privateRun(TestRunner.java:829)
	at org.testng.TestRunner.run(TestRunner.java:602)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:437)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:431)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:391)
	at org.testng.SuiteRunner.run(SuiteRunner.java:330)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:95)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1256)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1176)
	at org.testng.TestNG.runSuites(TestNG.java:1099)
	at org.testng.TestNG.run(TestNG.java:1067)
	at com.intellij.rt.testng.IDEARemoteTestNG.run(IDEARemoteTestNG.java:66)
	at com.intellij.rt.testng.RemoteTestNGStarter.main(RemoteTestNGStarter.java:105)

Modifications

Using pulsar.getWebServiceAddress() instead of pulsar.getSafeWebServiceAddress() pass to webServiceUrl when building LocalBrokerData/NamespaceEphemeralData/BrokerLookupData.

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

Documentation

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

Matching PR in forked repository

PR in forked repository:

@coderzc coderzc changed the title Fix returns wrong webServiceUrl when both webServicePort and webServicePortTls are set [fix][broker] Fix returns wrong webServiceUrl when both webServicePort and webServicePortTls are set Nov 28, 2023
@coderzc coderzc added this to the 3.2.0 milestone Nov 28, 2023
@coderzc coderzc added type/bug The PR fixed a bug or issue reported a bug area/broker labels Nov 28, 2023
@coderzc coderzc self-assigned this Nov 28, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 28, 2023
@Technoboy- Technoboy- closed this Nov 30, 2023
@Technoboy- Technoboy- reopened this Nov 30, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2023

Codecov Report

Merging #21633 (bbe4d0d) into master (98bf9dd) will decrease coverage by 0.30%.
Report is 31 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21633      +/-   ##
============================================
- Coverage     36.75%   36.46%   -0.30%     
+ Complexity    12215      782   -11433     
============================================
  Files          1715     1716       +1     
  Lines        131076   137700    +6624     
  Branches      14314    15457    +1143     
============================================
+ Hits          48182    50213    +2031     
- Misses        76498    80898    +4400     
- Partials       6396     6589     +193     
Flag Coverage Δ
inttests 24.27% <75.00%> (+0.09%) ⬆️
systests 24.79% <75.00%> (-0.13%) ⬇️
unittests 31.81% <87.50%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...che/pulsar/broker/loadbalance/NoopLoadManager.java 48.83% <100.00%> (ø)
...roker/loadbalance/impl/ModularLoadManagerImpl.java 69.72% <100.00%> (+2.29%) ⬆️
...broker/loadbalance/impl/SimpleLoadManagerImpl.java 50.51% <100.00%> (+0.12%) ⬆️
...apache/pulsar/broker/namespace/OwnershipCache.java 72.63% <100.00%> (ø)

... and 104 files with indirect coverage changes

@Technoboy- Technoboy- closed this Dec 5, 2023
@Technoboy- Technoboy- reopened this Dec 5, 2023
@Technoboy- Technoboy- merged commit f8067b5 into apache:master Dec 5, 2023
48 checks passed
lhotari pushed a commit that referenced this pull request Jan 19, 2024
…t and webServicePortTls are set (#21633)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit f8067b5)
lhotari pushed a commit that referenced this pull request Jan 19, 2024
…t and webServicePortTls are set (#21633)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit f8067b5)
lhotari added a commit that referenced this pull request Jan 26, 2024
…rvicePort and webServicePortTls are set (#21633)"

This reverts commit 91e073d.
lhotari pushed a commit that referenced this pull request Jan 26, 2024
…t and webServicePortTls are set (#21633)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit f8067b5)
lhotari added a commit that referenced this pull request Jan 26, 2024
…rvicePort and webServicePortTls are set (#21633)"

This reverts commit 5aedbdb.
lhotari pushed a commit that referenced this pull request Jan 26, 2024
…t and webServicePortTls are set (#21633)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit f8067b5)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
…t and webServicePortTls are set (apache#21633)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit f8067b5)
(cherry picked from commit 91e073d)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
…rvicePort and webServicePortTls are set (apache#21633)"

This reverts commit 91e073d.

(cherry picked from commit 5e0b424)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
…t and webServicePortTls are set (apache#21633)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit f8067b5)
(cherry picked from commit ba1f8a1)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
…t and webServicePortTls are set (apache#21633)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit f8067b5)
(cherry picked from commit 91e073d)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
…rvicePort and webServicePortTls are set (apache#21633)"

This reverts commit 91e073d.

(cherry picked from commit 5e0b424)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
…t and webServicePortTls are set (apache#21633)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit f8067b5)
(cherry picked from commit ba1f8a1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants