[BEAM-1542] Support ValueProviders in SpannerIO #3358
Conversation
|
R: @jkff |
|
|
||
| abstract long getBatchSizeBytes(); | ||
| @Nullable | ||
| abstract ValueProvider<Long> getBatchSizeBytes(); |
There was a problem hiding this comment.
Speaking of... Do you expect users to have to configure this in practice? Can SpannerIO choose a reasonable value and shield users from the temptation of "optimizing" it (which might fall out of date as Spanner implementation changes, or might be simply chosen misguidedly)?
There was a problem hiding this comment.
The default value of 1 MB is the optimal one. It is possible to disable batching by setting this to 0, so I think we should keep this.
There was a problem hiding this comment.
Okay, though consider making this a simpler-to-use builder method .withoutBatching().
Also: do you expect people to actually configure this via a ValueProvider? (I don't think we have guidelines on what builder methods should have a ValueProvider overload, but if we had such a guideline, it probably wouldn't be "all of them")
There was a problem hiding this comment.
Got it. I don't think batch size would be something that we let people configure in templates. Removed the ValueProvider.
|
We populate the display data, SpannerIO.java#L248. As long as ValueProvider implements toString it should work fine. |
|
Thanks, for display data, can you add a unit test and verify that it's formatted as expected? I think other IO connectors do a bit more to format ValueProvider's into DisplayData. |
|
Added display data test. |
|
retest this please |
No description provided.