-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 Pulsar Proxy to re-use authentication instance #12245
Conversation
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.
very well
can you add a unit test ?
Thanks for your contribution. For this PR, do we need to update docs? (The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks) |
Currently, the Pulsar Proxy creates a new PulsarClientImpl with a new instance of the client authentication plugin. For certain client auth implementations, this can cause issues. For example, if a client plugin needs to generate a token and then cache and re-use it (which is very common with typical Pulsar client usage) this pattern breaks, because the client auth plugin is tied to the lifecycle of the connection and not the more "singleton" usage of the Pulsar client. Arguably, we should instead figure out how to re-use the entire Pulsar client, but that likely has more complexity, instead this "quick fix" will get one of the most obvious cases solved.
It isn't clear why this test was doing timeouts... it doesn't really seem to be testing anything as the auth token was not being refreshed and it appears the test was expected to pass (it almost looks like they were expected to fail?) This allows us to make this test faster and more reliable, as the timeouts don't really seem to be adding anything
573bd3c
to
581e8af
Compare
Test has been added (via re-using existing test which wasn't very logical) |
/pulsarbot run-failure-checks |
* Fix Pulsar Proxy to re-use authentication instance Currently, the Pulsar Proxy creates a new PulsarClientImpl with a new instance of the client authentication plugin. For certain client auth implementations, this can cause issues. For example, if a client plugin needs to generate a token and then cache and re-use it (which is very common with typical Pulsar client usage) this pattern breaks, because the client auth plugin is tied to the lifecycle of the connection and not the more "singleton" usage of the Pulsar client. Arguably, we should instead figure out how to re-use the entire Pulsar client, but that likely has more complexity, instead this "quick fix" will get one of the most obvious cases solved. * add test for ensuring all same auth instance * Simplify ProxyAuthTest It isn't clear why this test was doing timeouts... it doesn't really seem to be testing anything as the auth token was not being refreshed and it appears the test was expected to pass (it almost looks like they were expected to fail?) This allows us to make this test faster and more reliable, as the timeouts don't really seem to be adding anything
* Fix Pulsar Proxy to re-use authentication instance Currently, the Pulsar Proxy creates a new PulsarClientImpl with a new instance of the client authentication plugin. For certain client auth implementations, this can cause issues. For example, if a client plugin needs to generate a token and then cache and re-use it (which is very common with typical Pulsar client usage) this pattern breaks, because the client auth plugin is tied to the lifecycle of the connection and not the more "singleton" usage of the Pulsar client. Arguably, we should instead figure out how to re-use the entire Pulsar client, but that likely has more complexity, instead this "quick fix" will get one of the most obvious cases solved. * add test for ensuring all same auth instance * Simplify ProxyAuthTest It isn't clear why this test was doing timeouts... it doesn't really seem to be testing anything as the auth token was not being refreshed and it appears the test was expected to pass (it almost looks like they were expected to fail?) This allows us to make this test faster and more reliable, as the timeouts don't really seem to be adding anything (cherry picked from commit 6b93a16)
* Fix Pulsar Proxy to re-use authentication instance Currently, the Pulsar Proxy creates a new PulsarClientImpl with a new instance of the client authentication plugin. For certain client auth implementations, this can cause issues. For example, if a client plugin needs to generate a token and then cache and re-use it (which is very common with typical Pulsar client usage) this pattern breaks, because the client auth plugin is tied to the lifecycle of the connection and not the more "singleton" usage of the Pulsar client. Arguably, we should instead figure out how to re-use the entire Pulsar client, but that likely has more complexity, instead this "quick fix" will get one of the most obvious cases solved. * add test for ensuring all same auth instance * Simplify ProxyAuthTest It isn't clear why this test was doing timeouts... it doesn't really seem to be testing anything as the auth token was not being refreshed and it appears the test was expected to pass (it almost looks like they were expected to fail?) This allows us to make this test faster and more reliable, as the timeouts don't really seem to be adding anything (cherry picked from commit 6b93a16) (cherry picked from commit ef27532)
* Fix Pulsar Proxy to re-use authentication instance Currently, the Pulsar Proxy creates a new PulsarClientImpl with a new instance of the client authentication plugin. For certain client auth implementations, this can cause issues. For example, if a client plugin needs to generate a token and then cache and re-use it (which is very common with typical Pulsar client usage) this pattern breaks, because the client auth plugin is tied to the lifecycle of the connection and not the more "singleton" usage of the Pulsar client. Arguably, we should instead figure out how to re-use the entire Pulsar client, but that likely has more complexity, instead this "quick fix" will get one of the most obvious cases solved. * add test for ensuring all same auth instance * Simplify ProxyAuthTest It isn't clear why this test was doing timeouts... it doesn't really seem to be testing anything as the auth token was not being refreshed and it appears the test was expected to pass (it almost looks like they were expected to fail?) This allows us to make this test faster and more reliable, as the timeouts don't really seem to be adding anything
Currently, the Pulsar Proxy creates a new PulsarClientImpl with a new
instance of the client authentication plugin.
For certain client auth implementations, this can cause issues. For
example, if a client plugin needs to generate a token and then cache and
re-use it (which is very common with typical Pulsar client usage) this
pattern breaks, because the client auth plugin is tied to the
lifecycle of the connection and not the more "singleton" usage of the
Pulsar client.
Arguably, we should instead figure out how to re-use the entire Pulsar
client, but that likely has more complexity, instead this "quick fix"
will get one of the most obvious cases solved.