SolaceIO: refactor to allow inheritance of BasicAuthSempClient #32400
Conversation
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
.../java/io/solace/src/main/java/org/apache/beam/sdk/io/solace/write/UnboundedSolaceWriter.java
Outdated
Show resolved
Hide resolved
| @@ -52,7 +56,7 @@ | |||
| * response is 401 Unauthorized, the client will execute an additional request with Basic Auth | |||
| * header to refresh the token. | |||
| */ | |||
| class SempBasicAuthClientExecutor implements Serializable { | |||
| public class SempBasicAuthClientExecutor implements Serializable { | |||
There was a problem hiding this comment.
This class doesn't need to be serialized, right?
There was a problem hiding this comment.
Can we leave it out for a separate PR?
There was a problem hiding this comment.
That's fine if this class isn't part of the public API, otherwise I'd fix it right now before users somehow introduce a dependency on the premise of serializability of this class.
There was a problem hiding this comment.
Edit, the tests are failing now. I think it has to be serializable, as this is a field in SempClient, which has to be serializable. I'll revert the changes
| @@ -81,6 +87,15 @@ class SempBasicAuthClientExecutor implements Serializable { | |||
| COOKIE_MANAGER_MAP.putIfAbsent(this.cookieManagerKey, new CookieManager()); | |||
There was a problem hiding this comment.
Not introduced by this PR, but this map may grow indefinitely. Also, this line unconditionally constructs a CookieManager. Could you change this to COOKIE_MANAGER_MAP.computeIfAbsent(this.cookieManagerKey, CookieManager::new) so it only constructs a new CookieManager when the key is absent?
There was a problem hiding this comment.
I'll change to COOKIE_MANAGER_MAP.computeIfAbsent(this.cookieManagerKey, CookieManager::new), thanks.
Regarding the unconditional growth - in theory yes, but in practice there won't be a big number of different combinations of baseUrls and usernames (CookieManagerKeys), so it won't grow beyond a few elements.
bzablocki
force-pushed
the
multiple-semp-refactor-and-example
branch
from
c009143 to
c6a7354
Compare
|
Reopening after an error with rebase |
bzablocki
force-pushed
the
multiple-semp-refactor-and-example
branch
from
a3694d1 to
eba116e
Compare
|
Run Java PreCommit |
bzablocki
force-pushed
the
multiple-semp-refactor-and-example
branch
from
c7a873c to
a0a4d3d
Compare
|
Run Java_Solace_IO_Direct PreCommit |
|
Run Java_Examples_Dataflow PreCommit |
|
@Abacn now that @sjvanrossum lgtm'ed, can I ask you for a review? |
Users were not able to create a custom BasicAuthSempClient due to some package-private fields. This PR fixes that and adds example on how to create and use a custom client.
Depends on #32060, waiting for that to be merged
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.