[BEAM-2803, BEAM-2706] JdbcIO improvements #3800
Conversation
| + "null data source"); | ||
| checkArgument(dataSource instanceof Serializable, | ||
| "DataSourceConfiguration.create(dataSource) called with a dataSource not Serializable"); | ||
| checkArgument(dataSource != null, "dataSource can not be null"); |
There was a problem hiding this comment.
Why not keeping the previous messages to be aligned with other IOs ?
There was a problem hiding this comment.
The style of messages is not really consistent between all IOs and I'm changing this one to be according to the best practices recently discussed on dev@ and currently recommended by PT Style Guide https://beam.apache.org/contribute/ptransform-style-guide/#validation
| return toBuilder().setCoder(coder).build(); | ||
| } | ||
|
|
||
| @Override | ||
| public PCollection<T> expand(PBegin input) { | ||
| checkArgument(getQuery() != null, "withQuery() is required"); |
There was a problem hiding this comment.
Do we need these checkArgument()s here as it's checked in the create ?
There was a problem hiding this comment.
withQuery() checks that you don't intentionally pass null to it; expand() checks that you didn't forget to call the withQuery() method at all.
| @@ -253,6 +252,43 @@ public void setParameters(PreparedStatement preparedStatement) | |||
| pipeline.run(); | |||
| } | |||
|
|
|||
| @Test | |||
| @Category(NeedsRunner.class) | |||
There was a problem hiding this comment.
AFAIR NeedsRunner is not required there.
There was a problem hiding this comment.
Done, thanks, removed it from other tests too!
jkff
force-pushed
the
jdbcio-improvements
branch
from
e5daf60
to
bc9723a
Compare
|
Thanks, PTAL. |
|
retest this please |
1 similar comment
|
retest this please |
jkff
force-pushed
the
jdbcio-improvements
branch
from
bc9723a
to
e5daf60
Compare
jkff
force-pushed
the
jdbcio-improvements
branch
from
e5daf60
to
1476a3f
Compare
This solves two issues:
Also brings the validation messages in accordance with the updated guidance in PTransform Style Guide.
R: @jbonofre