-
Notifications
You must be signed in to change notification settings - Fork 518
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
Changes of incremental import billing #3354
Changes of incremental import billing #3354
Conversation
@@ -145,6 +146,103 @@ public async Task GivenIncrementalLoad_MultipleInputVersions_ResourceNotExisting | |||
Assert.Equal(GetLastUpdated("2002"), result.Resource.Meta.LastUpdated); | |||
} | |||
|
|||
[Fact] | |||
public async Task GivenIncrementalImportInvalidResource_ThenErrorLogsShouldBeOutput_FailedRCountShouldMatch() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Is the R in the test title a typo or suppose to be the work Resource? Also, the test name should be in Given/When/Then format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LTA-Thinking Yes, type. Changed to GivenIncrementalImportInvalidResource__WhenImportData_ThenErrorLogsShouldBeOutputAndFailedCountShouldMatch.
/// <summary> | ||
/// Import mode. | ||
/// </summary> | ||
public ImportMode ImportMode { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not add ImportMode here as it is available in the job Definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SergeyGaluzo removed
JobInfo contains Status, Result and Definition. Looks some parameters are redundant. Refers to: src/Microsoft.Health.Fhir.SqlServer/Features/Operations/Import/ImportOrchestratorJob.cs:268 in c55f771. [](commit_id = c55f771, deletion_comment = False) |
|
||
public int CreatedJobCount { get; set; } // TODO: remove in stage 3 | ||
|
||
public long? TotalSizeInBytes { get; set; } // TODO: remove in stage 3 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you did not remove these 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e352979
Description
Changes to add billing to incremental import.
ImportJobMetricsNotification needs to be published with ImportMode information so on Health PaaS side, we will know to bill the import requests or not. This PR has changes to add ImportMode in the notification that will be handled in Handler that is on HP side.
Related issues
Addresses [issue 103222].
Testing
Unit tests added
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)