Add mechanism to override bigtable client for testing #28374
Conversation
jackdingilian
force-pushed
the
cdc-testing
branch
from
e77077d to
13b12da
Compare
|
LGTM |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| /** | ||
| * Returns a new {@link BigtableIO.ReadChangeStream} that overrides the config of data and/or | ||
| * admin client for streaming changes and for managing the metadata. For testing purposes only. | ||
| * Not intended for use. |
There was a problem hiding this comment.
Maybe this is useful also for regional endpoints or some such? (no action required - it is certainly always easier to make things more visible than less visible)
|
|
||
| @VisibleForTesting | ||
| BigtableConfig withBigtableClientOverride(BigtableClientOverride clientOverride) { | ||
| checkArgument(clientOverride != null, "clientOverride can not be null"); |
There was a problem hiding this comment.
If someone has set the override and you want to clear it, how do you do that?
There was a problem hiding this comment.
good point, I guess this check isn't really necessary. I'll remove it
There was a problem hiding this comment.
on second thought, all of the other withParameter functions here have similar null checks that wouldn't allow overrides.
I'm leaning towards keeping this to be consistent. I don't think there's a good reason to ever set this and then clear it.
There was a problem hiding this comment.
OK that seems fine. It is backwards-compatible to start to allow nulls as a way to re-clear it. One reason to allow nulls is to allow someone to make a copy without forcing them to if (!= null) { set it } but just be able to do straight-line set to same value, whether or not it is null
| * | ||
| * <p>Does not modify this object. | ||
| */ | ||
| @VisibleForTesting |
There was a problem hiding this comment.
It says visible for testing, but there are no tests. It seems like you can test this by setting up the PipelineOptions in the test before you expand the BigTableChangeStreamsAccessor
There was a problem hiding this comment.
IIUC this would require exposing all of the override parameters as PipelineOptions to users? We use this to connect to our internal testing environment. It requires fairly complicated client settings overrides that I don't think would ever be useful to expose to users
There was a problem hiding this comment.
Oh I misread the whole change and thought there were already options for this (those do tend to be useful for redirecting a whole test at a different bigtable env or local endpoint). Nevermind for now!
|
|
||
| @VisibleForTesting | ||
| BigtableConfig withBigtableClientOverride(BigtableClientOverride clientOverride) { | ||
| checkArgument(clientOverride != null, "clientOverride can not be null"); |
There was a problem hiding this comment.
OK that seems fine. It is backwards-compatible to start to allow nulls as a way to re-clear it. One reason to allow nulls is to allow someone to make a copy without forcing them to if (!= null) { set it } but just be able to do straight-line set to same value, whether or not it is null
| * | ||
| * <p>Does not modify this object. | ||
| */ | ||
| @VisibleForTesting |
There was a problem hiding this comment.
Oh I misread the whole change and thought there were already options for this (those do tend to be useful for redirecting a whole test at a different bigtable env or local endpoint). Nevermind for now!
3 participants
Adds
BigtableClientOverrideinterface to provide a way to override client settings we need to change for testing.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.