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

Batch size dependent retries for $import #4668

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Conversation

SergeyGaluzo
Copy link
Contributor

@SergeyGaluzo SergeyGaluzo commented Oct 10, 2024

@SergeyGaluzo SergeyGaluzo added Enhancement Enhancement on existing functionality. Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Schema Version unchanged labels Oct 10, 2024
@SergeyGaluzo SergeyGaluzo added this to the S151 milestone Oct 10, 2024
@SergeyGaluzo SergeyGaluzo requested a review from a team as a code owner October 10, 2024 15:04
}

ExecuteSql("TRUNCATE TABLE EventLog");
ExecuteSql("DELETE FROM Resource");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this going to affect other E2E tests since they run in parallel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

_logger.LogWarning(e, $"Error on {nameof(ImportResourcesInternalAsync)} retries={{Retries}}", retries);
await Task.Delay(1000, cancellationToken);
_logger.LogWarning(e, $"Error on {nameof(ImportResourcesInternalAsync)} retries={{Retries}} resources={{Resources}}", retries, resources.Count);
await Task.Delay(retries > 3 ? 10 : 1000, cancellationToken); // if >3 assume that it is id generation problem
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a different / more specific SQL error signal help us determine the reason for the conflict vs assuming on the number of retries?

Copy link
Contributor Author

@SergeyGaluzo SergeyGaluzo Oct 10, 2024

Choose a reason for hiding this comment

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

Yes. It would. This is more work than I want to do, and large number of retries indicates id collision with very high accuracy. This is true especially for $import that runs internal deduplication.

@microsoft microsoft deleted a comment from mikaelweave Oct 10, 2024
@microsoft microsoft deleted a comment from azure-pipelines bot Oct 10, 2024
@microsoft microsoft deleted a comment from azure-pipelines bot Oct 10, 2024
@SergeyGaluzo SergeyGaluzo merged commit b7b781e into main Oct 10, 2024
47 checks passed
@SergeyGaluzo SergeyGaluzo deleted the users/sergal/importretries branch October 10, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Enhancement Enhancement on existing functionality. Schema Version unchanged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants