aromanenko-dev edited a comment on pull request #14856: URL: https://github.com/apache/beam/pull/14856#issuecomment-907293686
@AydarZaynutdinov Thank you for working on this! > - rename method withReturningResults() to withWriteResults(). > - keep old method with name withResults() (do not rename it to withVoidResults). > - change back private to public access for writeVoid method. Right. > records by batches. Seems like Vitaliy explained why there is an execute() method instead of executeBatch(). So I guess I should leave it like it is now -> use execute(). Is it right? Yes, if there is no other way to send batched records for better performance. > wrap up V. I'm not sure that we need this wraping, because a new abstract class JdbcWriteResult will have no any rules or parameters and as a result it would be just an empty class. Maybe it would be better to transform JdbcWriteResult to empty interface? So it would be like a marker for each V. Could you please share your thoughts? Yes but I'd suggest to create an abstract class `JdbcWriteResult`, like we do for `BigtableIO` and `BigtableWriteResult`, to be consistent with other IOs in this case. > Is there anything else I should address? Please, don't merge `master` branch into the feature branch. Instead, just rebase your branch with latest `master`. Also, this [guide](https://beam.apache.org/contribute/) could be helpful just in case if you haven't seen it yet. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
