Skip to content
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

Retry Wrapper | WIP #19

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Retry Wrapper | WIP #19

wants to merge 1 commit into from

Conversation

psainics
Copy link

@psainics psainics commented Aug 21, 2024

Generating diff for branch: [feat/bulk-api-retry] against [develop]

Default Title

Jira : PLUGIN-0000

Description

Default Description

UI Field

  • Modified Salesforce-batchsink.json

Docs

  • No Changes made to docs.

Code change

  • Added BulkConnectionRetryWrapper.java
  • Modified SalesforceBulkUtil.java
  • Modified SalesforceOutputFormatProvider.java
  • Modified SalesforceSinkConfig.java
  • Modified SalesforceBatchMultiSource.java
  • Modified SalesforceBatchSource.java
  • Modified SalesforceBulkRecordReader.java
  • Modified SalesforceSplitUtil.java

Unit Tests

  • No Changes made to unit tests.

@psainics psainics added the enhancement New feature or request label Aug 21, 2024
@@ -65,8 +66,8 @@ public final class SalesforceBulkUtil {
* @throws AsyncApiException if there is an issue creating the job
*/

public static JobInfo createJob(BulkConnection bulkConnection, String sObject, OperationEnum operationEnum,
@Nullable String externalIdField,
public static JobInfo createJob(BulkConnectionRetryWrapper bulkConnection, String sObject,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename the field to bulkConnectionRetryWrapper

@@ -164,7 +165,10 @@ public static List<SalesforceSplit> getSplits(
bulkConnection.addHeader(SalesforceSourceConstants.HEADER_ENABLE_PK_CHUNK,
String.join(";", chunkHeaderValues));
}
List<SalesforceSplit> querySplits = SalesforceSplitUtil.getQuerySplits(query, bulkConnection,
BulkConnectionRetryWrapper bulkConnectionRetryWrapper = new BulkConnectionRetryWrapper(bulkConnection,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than passing the bulkConnection here, write a method in SalesforceSplitUtil that returns RetryWrapper

}

public JobInfo createJob(JobInfo jobInfo) throws AsyncApiException {
if (!retryOnBackendError) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than adding if condition for each method, can we have a retry policy that dont try after failure. E.g maxAttempts = 1

try {
return createBatchFromStream(query, job);
} catch (AsyncApiException e) {
throw new SalesforceQueryExecutionException(e.getMessage());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SalesforceQueryExecutionException will only be thrown if error code is among the four which we were using previously, write a method that will handle AsyncAPIException and throw error only if code is among those and call that from all catches

@psainics
Copy link
Author

As per new guidelines please get design doc approved before reviewing the PR. 😄 ✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants