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 fuctionality for all bulk apis #18

Open
wants to merge 6 commits into
base: release/1.6
Choose a base branch
from

Conversation

vikasrathee-cs
Copy link
Collaborator

@vikasrathee-cs vikasrathee-cs commented Aug 8, 2024

@psainics We need to move these classes on top of develop branch instead of 1.6. I am adding few suggestions for you to do.

@vikasrathee-cs vikasrathee-cs assigned psainics and unassigned psainics Aug 8, 2024
pom.xml Outdated
@@ -66,7 +66,7 @@
<spark2.version>2.1.3</spark2.version>
<hydrator.version>2.10.0</hydrator.version>
<commons.version>3.8.1</commons.version>
<salesforce.api.version>53.0.0</salesforce.api.version>
<salesforce.api.version>61.0.0</salesforce.api.version>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

raise a separate PR just for this version upgrade.

Choose a reason for hiding this comment

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

@@ -68,6 +69,7 @@ public final class SalesforceBulkUtil {
public static JobInfo createJob(BulkConnection bulkConnection, String sObject, OperationEnum operationEnum,
@Nullable String externalIdField,
ConcurrencyMode concurrencyMode, ContentType contentType) throws AsyncApiException {
BulkConnectionRetryWrapper bulkConnectionRetryWrapper = new BulkConnectionRetryWrapper(bulkConnection);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of creating object of RetryWrapper, create it once in getBulkConnection method and rename it and return wrapper instead of bulkConnection. Retry criteria fields also fetch from config instead of hard coding as given now

Choose a reason for hiding this comment

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

Retry criteria fields also fetch from config instead of hard coding as given now

Which config ?
There are no fields for retry

return (InputStream) inputStream;
}

public static RetryPolicy<Object> getRetryPolicy(Long initialRetryDuration, Long maxRetryDuration,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use the one from SplitUtil class and call it once while creating retry wrapper. If retryDisabled, handle it in this class constructor.

import io.cdap.plugin.salesforce.plugin.source.batch.util.SalesforceQueryExecutionException;
import io.cdap.plugin.salesforce.plugin.source.batch.util.SalesforceSourceConstants;
import io.cdap.plugin.salesforce.plugin.source.batch.util.SalesforceSplitUtil;

import org.apache.commons.csv.CSVFormat;
import org.apache.commons.csv.CSVParser;
import org.apache.commons.csv.CSVRecord;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some of the methods are still not called using retry wrapper. E.g. line 208. move those also to retry wrapper class

.get(() -> bulkConnection.updateJob(jobInfo));
}

public BatchInfoList getBatchInfoList(String jobId) throws AsyncApiException {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add try catch to each method and in catch add a method that handles asyncapiException and handle retry as retry will be happen based on SalesforceQueryExecutionException.

Choose a reason for hiding this comment

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

Added !

@psainics
Copy link

Ref : #19

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

Successfully merging this pull request may close these issues.

2 participants