Skip to content

Comments

[fix][client] Fix authentication not update after changing the serviceUrl#19510

Merged
nodece merged 1 commit intoapache:masterfrom
hangc0276:chenhang/fix_authentication_not_update_after_changing_the_serviceUrl
Feb 14, 2023
Merged

[fix][client] Fix authentication not update after changing the serviceUrl#19510
nodece merged 1 commit intoapache:masterfrom
hangc0276:chenhang/fix_authentication_not_update_after_changing_the_serviceUrl

Conversation

@hangc0276
Copy link
Contributor

@hangc0276 hangc0276 commented Feb 14, 2023

Motivation

When we use cluster level failover and enabled authentication, we found the authentication couldn't update in the HttpLookupService. The root cause is that the HttpLookupService cached the httpClient, but the updateServiceUrl only updates the target serviceUrl for the httpClient, without any updates for authentication.

public HttpLookupService(ClientConfigurationData conf, EventLoopGroup eventLoopGroup)
throws PulsarClientException {
this.httpClient = new HttpClient(conf, eventLoopGroup);
this.useTls = conf.isUseTls();
this.listenerName = conf.getListenerName();
}
@Override
public void updateServiceUrl(String serviceUrl) throws PulsarClientException {
httpClient.setServiceUrl(serviceUrl);
}

Modifications

Call pulsarClient.reloadLookUp() to update the authentication manually when the target cluster is switched.

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 the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: hangc0276#10

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Feb 14, 2023
@hangc0276 hangc0276 self-assigned this Feb 14, 2023
@codelipenghui codelipenghui added this to the 3.0.0 milestone Feb 14, 2023
@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/client labels Feb 14, 2023
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@hangc0276 hangc0276 changed the title Fix authentication not update after changing the serviceUrl [fix][client] Fix authentication not update after changing the serviceUrl Feb 14, 2023
@nodece nodece merged commit 0f025f3 into apache:master Feb 14, 2023
@momo-jun
Copy link
Contributor

Hi @hangc0276, did you have any plans to update the docs for this fix? Just want to make sure it can catch up with the release schedule.

@hangc0276 hangc0276 added doc-not-needed Your PR changes do not impact docs and removed doc-required Your PR changes impact docs and you will update later. labels Feb 21, 2023
@hangc0276
Copy link
Contributor Author

Hi @hangc0276, did you have any plans to update the docs for this fix? Just want to make sure it can catch up with the release schedule.

@momo-jun sorry, I add the wrong doc tag. This Pr doesn't need any doc updates.

liangyepianzhou pushed a commit that referenced this pull request Feb 25, 2023
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Feb 28, 2023
…eUrl (apache#19510)

(cherry picked from commit 0f025f3)
(cherry picked from commit 132abe9)
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.

8 participants