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
[BEAM-3008] Adds BigtableOptions configurator to the BigtableIO #4205
Conversation
- Deprecates previous BigtableOptions providing methods.
* .setProjectId("project") | ||
* .setInstanceId("instance") | ||
* .withProjectId("project") | ||
* .withInstanceId("instance") | ||
* .withTableId("table")); | ||
* }</pre> | ||
* | ||
* <h3>Using local emulator</h3> |
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.
Can you please remove the whole Using local emulator section. It's not right anymore.
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.
I'll approve. We need to work on toString and display data, but we can only do that once we do more consolidation of logic around instance/project/bigtableoptions/bigtableoptions Function
@@ -438,6 +474,8 @@ public String toString() { | |||
.add("tableId", getTableId()) | |||
.add("keyRange", getKeyRange()) | |||
.add("filter", getRowFilter()) | |||
.add("bigtableOptionsConfiguratorProvided", |
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.
It's worth printing out the options themselves post configurator. Printing the configurator class might be worth while as well.
- Adds iinfo to toString and populateDisplayData about effective BigtableOptions
@sduskis I have organized them for now in single function but it caused the code duplication, once we move it to the BigtableConfig, it would be much nicer. I am not sure that we would be able to populate toString with data once we switch to ValueProviders because we might not have values in time when toString might be called, for populate display data should be fine. |
Hi @chamikaramj, please merge this PR, @sduskis approved it. Thanks. |
LGTM. Merging. |
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue.mvn clean verify
to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.