[BEAM-9742] Add Configurable FluentBackoff to JdbcIO Write #11396

Akshay-Iyangar · 2020-04-11T06:14:10Z

Add Configurable FluentBackoff to JdbcIO Write
Jira ticket

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

Akshay-Iyangar · 2020-04-11T06:15:01Z

@aromanenko-dev @iemejia @lukecwik

lukecwik · 2020-04-15T17:21:24Z

sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java

+ * This is the default {@link FluentBackoffConfiguration} that we use to retry when a {@link
+ * SQLException} occurs.
+ */
+ public static class DefaultFluentBackoffConfiguration implements FluentBackoffConfiguration {


We can't make FluentBackoff part of the public API surface since it is in the util package. It looks like other IO connectors have been creating their own RetryConfiguration class such as:
SnsIO:

beam/sdks/java/io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/sns/SnsIO.java

Line 262 in da9e172

public Write<T> withRetryConfiguration(RetryConfiguration retryConfiguration) {

SolrIO:

beam/sdks/java/io/solr/src/main/java/org/apache/beam/sdk/io/solr/SolrIO.java

Line 225 in da9e172

public abstract static class RetryConfiguration implements Serializable {

and then converting them to any internal implementation that makes sense.

Making FluentBackoff or a RetryConfiguration public and shared across implementation might make sense but warrants a discussion on the dev@ mailing list to see what the community thinks.

@lukecwik - cool! I will mail the dev group.

Waiting on updates on ML thread:
https://lists.apache.org/thread.html/r7fde7b5c87c6008689a013fc113d869d3ac15cbc7a35e4534469b9ab%40%3Cdev.beam.apache.org%3E

It looks like we should stick with the pattern other IO authors have taken in the mean time.

I agree with @lukecwik - we should stay consistent with that.

Akshay-Iyangar · 2020-05-06T21:31:40Z

@lukecwik @aromanenko-dev @jfarr @timrobertson100 Let me know what you guys think needs to be done.
Seems like people are ok with making FluentBackOff public?

aromanenko-dev · 2020-05-07T16:12:23Z

Well, I didn't see any principal "against" to make FluentBackoff public but I'm not sure that it will be moved to another package or util will become public soon. So for now, I'd rather keep the same strategy as we did for other IOs by creating a custom RetryConfiguration for certain IO.
@lukecwik wdyt?

lukecwik · 2020-05-07T21:07:12Z

That makes sense to me. We can always expose it in the future and deprecate existing IO specific versions.

…

On Thu, May 7, 2020 at 9:12 AM Alexey Romanenko ***@***.***> wrote: Well, I didn't see any principal "against" to make FluentBackoff public but I'm not sure that it will be moved to another package or util will become public soon. So for now, I'd rather keep the same strategy as we did for other IOs by creating a custom RetryConfiguration for certain IO. @lukecwik <https://github.com/lukecwik> wdyt? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11396 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACM4V3BEC7AEJ5AVMDDZ52LRQLMXRANCNFSM4MF4BJ7A> .

lukecwik · 2020-05-07T21:07:21Z

Well, I didn't see any principal "against" to make FluentBackoff public but I'm not sure that it will be moved to another package or util will become public soon. So for now, I'd rather keep the same strategy as we did for other IOs by creating a custom RetryConfiguration for certain IO.
@lukecwik wdyt?

That makes sense to me. We can always expose it in the future and deprecate existing IO specific versions.

udim · 2020-05-08T19:47:28Z

Hi, Cham requested that I take a look but I'm overloaded. Will attempt to look next week

aromanenko-dev · 2020-05-11T11:20:53Z

@udim no problem, I can take over of this PR

aromanenko-dev · 2020-05-11T11:22:45Z

Run Java PreCommit

aromanenko-dev

@Akshay-Iyangar Following up the discussion here and on email thread, I propose to stay consistent with the RetryConfiguration pattern that was already used in other IO. Please, address this comments.

Also, please, rebase your feature branch against current master to avoid the merge commits there.

aromanenko-dev · 2020-05-11T11:37:47Z

sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java

+ * This is the default {@link FluentBackoffConfiguration} that we use to retry when a {@link
+ * SQLException} occurs.
+ */
+ public static class DefaultFluentBackoffConfiguration implements FluentBackoffConfiguration {


I agree with @lukecwik - we should stay consistent with that.

aromanenko-dev · 2020-05-26T08:48:15Z

Hi @Akshay-Iyangar, kind ping regarding this PR.

Akshay-Iyangar · 2020-05-26T20:30:22Z

@aromanenko-dev - Sorry for a little busy, will make the changes today.

Akshay-Iyangar · 2020-06-02T21:59:13Z

@aromanenko-dev @lukecwik could you'll please have a look? Thanks

aromanenko-dev

Thanks, it's almost LGTM. I left a couple of minor notes.

sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java

aromanenko-dev · 2020-06-04T17:04:17Z

retest this please

Akshay-Iyangar · 2020-06-08T08:56:09Z

@aromanenko-dev - Addressed the feedback !!

aromanenko-dev · 2020-06-08T12:08:42Z

retest this please

aromanenko-dev · 2020-06-08T12:18:43Z

Run Java PreCommit

aromanenko-dev · 2020-06-08T13:26:02Z

Run Java PreCommit

aromanenko-dev · 2020-06-08T14:55:38Z

Run Java PreCommit

aromanenko-dev

Thanks, it LGTM. I'm going to merge it once Java PreCommit will be green.
@lukecwik Do you have something to add?

aromanenko-dev · 2020-06-08T17:14:15Z

Run Java PreCommit

Akshay-Iyangar · 2020-06-08T19:25:08Z

Run Java PreCommit

aromanenko-dev · 2020-06-09T09:30:29Z

Run Java PreCommit

aromanenko-dev · 2020-06-10T08:52:26Z

Run Java PreCommit

aromanenko-dev · 2020-06-10T15:47:36Z

retest this please

aromanenko-dev · 2020-06-10T18:42:01Z

I'm going to merge this PR since the failed test org.apache.beam.sdk.io.TextIOWriteTest.testWriteViaSink is not related to this change. All JdbcIO-related teste passed.

) * [BEAM-9742] revert back to the older version * [BEAM-9742] Using retry configuration for fluent-backoff * [BEAM-9742] fixing checkstyle errors. * [BEAM-9742] Addressing feedback Co-authored-by: aiyangar <[email protected]>

probot-autolabeler bot added io java jdbc labels Apr 11, 2020

lukecwik requested changes Apr 15, 2020

View reviewed changes

chamikaramj requested a review from udim May 8, 2020 05:05

aromanenko-dev requested changes May 11, 2020

View reviewed changes

probot-autolabeler bot added build infra labels Jun 2, 2020

Akshay-Iyangar closed this Jun 2, 2020

Akshay-Iyangar force-pushed the addBatchRetries branch from 2364b71 to 01c11e7 Compare June 2, 2020 06:32

[BEAM-9742] revert back to the older version

ed6e349

Akshay-Iyangar reopened this Jun 2, 2020

aiyangar-godaddy added 2 commits June 2, 2020 14:52

[BEAM-9742] Using retry configuration for fluent-backoff

65c677a

[BEAM-9742] fixing checkstyle errors.

17abcd7

aromanenko-dev reviewed Jun 4, 2020

View reviewed changes

sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java Show resolved Hide resolved

sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java Outdated Show resolved Hide resolved

[BEAM-9742] Addressing feedback

a343063

aromanenko-dev approved these changes Jun 8, 2020

View reviewed changes

aromanenko-dev merged commit 837b52f into apache:master Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-9742] Add Configurable FluentBackoff to JdbcIO Write #11396

[BEAM-9742] Add Configurable FluentBackoff to JdbcIO Write #11396

Akshay-Iyangar commented Apr 11, 2020

Akshay-Iyangar commented Apr 11, 2020

lukecwik Apr 15, 2020

Akshay-Iyangar Apr 15, 2020

lukecwik Apr 16, 2020

lukecwik May 7, 2020

aromanenko-dev May 11, 2020

Akshay-Iyangar commented May 6, 2020

aromanenko-dev commented May 7, 2020

lukecwik commented May 7, 2020 via email

lukecwik commented May 7, 2020

udim commented May 8, 2020

aromanenko-dev commented May 11, 2020

aromanenko-dev commented May 11, 2020

aromanenko-dev left a comment •

edited

Loading

aromanenko-dev May 11, 2020

aromanenko-dev commented May 26, 2020

Akshay-Iyangar commented May 26, 2020

Akshay-Iyangar commented Jun 2, 2020

aromanenko-dev left a comment

aromanenko-dev commented Jun 4, 2020

Akshay-Iyangar commented Jun 8, 2020

aromanenko-dev commented Jun 8, 2020

aromanenko-dev commented Jun 8, 2020

aromanenko-dev commented Jun 8, 2020

aromanenko-dev commented Jun 8, 2020

aromanenko-dev left a comment

aromanenko-dev commented Jun 8, 2020

Akshay-Iyangar commented Jun 8, 2020

aromanenko-dev commented Jun 9, 2020

aromanenko-dev commented Jun 10, 2020

aromanenko-dev commented Jun 10, 2020

aromanenko-dev commented Jun 10, 2020

[BEAM-9742] Add Configurable FluentBackoff to JdbcIO Write #11396

[BEAM-9742] Add Configurable FluentBackoff to JdbcIO Write #11396

Conversation

Akshay-Iyangar commented Apr 11, 2020

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Akshay-Iyangar commented Apr 11, 2020

lukecwik Apr 15, 2020

Choose a reason for hiding this comment

Akshay-Iyangar Apr 15, 2020

Choose a reason for hiding this comment

lukecwik Apr 16, 2020

Choose a reason for hiding this comment

lukecwik May 7, 2020

Choose a reason for hiding this comment

aromanenko-dev May 11, 2020

Choose a reason for hiding this comment

Akshay-Iyangar commented May 6, 2020

aromanenko-dev commented May 7, 2020

lukecwik commented May 7, 2020 via email

lukecwik commented May 7, 2020

udim commented May 8, 2020

aromanenko-dev commented May 11, 2020

aromanenko-dev commented May 11, 2020

aromanenko-dev left a comment • edited Loading

Choose a reason for hiding this comment

aromanenko-dev May 11, 2020

Choose a reason for hiding this comment

aromanenko-dev commented May 26, 2020

Akshay-Iyangar commented May 26, 2020

Akshay-Iyangar commented Jun 2, 2020

aromanenko-dev left a comment

Choose a reason for hiding this comment

aromanenko-dev commented Jun 4, 2020

Akshay-Iyangar commented Jun 8, 2020

aromanenko-dev commented Jun 8, 2020

aromanenko-dev commented Jun 8, 2020

aromanenko-dev commented Jun 8, 2020

aromanenko-dev commented Jun 8, 2020

aromanenko-dev left a comment

Choose a reason for hiding this comment

aromanenko-dev commented Jun 8, 2020

Akshay-Iyangar commented Jun 8, 2020

aromanenko-dev commented Jun 9, 2020

aromanenko-dev commented Jun 10, 2020

aromanenko-dev commented Jun 10, 2020

aromanenko-dev commented Jun 10, 2020

aromanenko-dev left a comment •

edited

Loading