-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[improve][client] Implement HTTP client using javax.ws.rs #23905
base: master
Are you sure you want to change the base?
Conversation
This would be a major change to the admin client. It feels that there's something wrong in the AuthenticationSasl if it gets coupled to the client type.
What does "maintain the TLS context" mean in practice? What are the different implementation options? |
One detail to consider is that we'd like to migrate off |
Should be client, not admin client?
This authentication process requires the broker endpoint, and we pass the URL string, which indicates to the broker which endpoint is being requested.
Just pass the WebTarget to the Authtication. I will make a new PR to handle this thing.
I think we can only pass the WebTarget to the auth. Do you any ideas? Using jakarta.ws.rs instead of javax.ws.rs is a good idea, but fixing the bug is still important, so I want to keep this path. |
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
1be3d96
to
e1abde2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching the HTTP client in pulsar-client-admin is a major change and it needs a PIP to document the change. We should also ensure that the performance is similar. While changing the client, we should also get rid of the async-http-client dependency.
Motivation
I encountered an issue when using Kerberos and TLS transport to connect to the broker. The
pulsar-admin
andpulsar-client
(for HTTP lookup) failed to work as expected.In the
org.apache.pulsar.client.impl.auth.AuthenticationSasl#authenticationStage
method, the broker requests should use theWebTarget
provided by thepulsar-admin
orpulsar-client
HTTP clients. However, the current implementation creates a new HTTP client to construct theWebTarget
, causing the TLS context to be lost.Currently, the
pulsar-admin
HTTP client is built onjavax.ws.rs
and AHC (Async Http Client), while thepulsar-client
HTTP client relies only on AHC. To resolve this inconsistency and maintain the TLS context, we need to refactor thepulsar-client
to provide the requiredWebTarget
.Modifications
Package Changes:
org.apache.pulsar.client.admin.internal.http
toorg.apache.pulsar.client.internal.http
.pulsar-client-admin
topulsar-client
.Refactored HTTP Client:
HttpClient.java
to useAsyncHttpConnectorProvider
andAsyncHttpConnector
for a unified and consistent implementation.Fixed Client Calls:
pulsar-client
calls to utilize the refactored HTTP client.Documentation
doc
doc-required
doc-not-needed
doc-complete