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.

Sign up for GitHub

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

Jump to bottom

Moves Cosmos Export to QueueClient #3169

Merged
merged 4 commits into from
Jun 5, 2023
Merged

Moves Cosmos Export to QueueClient #3169

merged 4 commits into from
Jun 5, 2023

Conversation

brendankowitz
Copy link
Member

@brendankowitz brendankowitz commented Mar 10, 2023 •
edited
Loading

Description

  • Moves Cosmos Export to QueueClient
  • Existing Cosmos Export jobs will continue to run under the old model (LegacyExportJobWorkerBackgroundService) to avoid disruption or file name changes

Warning: File names in Cosmos export will change for new jobs.

Related issues

Addresses #3248

Testing

Existing export tests
Manual testing

  • Queueing jobs in main, checking out and running in this branch for continuity

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@brendankowitz brendankowitz added KI-Warning This is a known issue that causes a warning that customers should be aware of Enhancement-Refactor Refactor on existing functionality. Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR labels Mar 10, 2023
@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch 3 times, most recently from dc7d171 to 58e89f6 Compare March 12, 2023 18:13
@brendankowitz brendankowitz added this to the S111 milestone Mar 12, 2023
@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch from 58e89f6 to cb4b073 Compare March 13, 2023 18:13
@brendankowitz brendankowitz marked this pull request as ready for review March 14, 2023 00:01
@brendankowitz brendankowitz requested a review from a team as a code owner March 14, 2023 00:01
@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch from cb4b073 to 4b50970 Compare March 21, 2023 23:13
@LTA-Thinking
Copy link
Collaborator

I think the legacy export code should be removed before merging this in. Having two code paths active at once will just create confusing code flow and unpredictable functionality. I've tested this locally without it and it works. I don't see any reason why it would still be needed.

LTA-Thinking
LTA-Thinking reviewed Apr 4, 2023
View reviewed changes
src/Microsoft.Health.Fhir.CosmosDb/Features/Operations/Export/CosmosExportOrchestratorJob.cs Outdated Show resolved Hide resolved
...osoft.Health.Fhir.SqlServer.UnitTests/Features/Operations/Export/ExportProcessingJobTests.cs Outdated Show resolved Hide resolved
src/Microsoft.Health.Fhir.SqlServer/Features/Storage/ResourceSurrogateIdHelper.cs Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this file be removed? It doesn't look like it is doing any work, it is just pass through methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, however removing it in this PR will probably expand the affected files. I was thinking to do it in a subsequent PR
image

@LTA-Thinking
Copy link
Collaborator

The updated code doesn't seem able to find export jobs made the old way when a user tries to hit the job status endpoint.

@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch 2 times, most recently from ecbfe92 to 73d3793 Compare April 5, 2023 20:00
@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch 2 times, most recently from dbc955d to 2650b1d Compare April 18, 2023 00:58
@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch 3 times, most recently from d627663 to 56c6601 Compare April 27, 2023 17:01
@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch 2 times, most recently from 073fa9d to d80336c Compare May 11, 2023 17:48
@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch from d80336c to 0a7d4fd Compare May 17, 2023 22:19
@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch 2 times, most recently from 4587405 to a6a5360 Compare May 26, 2023 22:22
@LTA-Thinking LTA-Thinking mentioned this pull request Jun 1, 2023
Merged
1 task
@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-queueclient-export3 branch from a6a5360 to 98f30e6 Compare June 1, 2023 14:48
LTA-Thinking
LTA-Thinking previously approved these changes Jun 1, 2023
View reviewed changes
@brendankowitz brendankowitz merged commit e261f9c into main Jun 5, 2023
@brendankowitz brendankowitz deleted the personal/bkowitz/cosmos-queueclient-export3 branch June 5, 2023 17:02
PTaladay added a commit that referenced this pull request Jun 6, 2023
@PTaladay
Adding FhirOperationDataStoreBase from #3169 as I need to build off o…
6326e3b 
…f it and the PR isn't complete yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PTaladay PTaladay PTaladay left review comments

@LTA-Thinking LTA-Thinking LTA-Thinking approved these changes

Assignees
No one assigned
Labels
Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Enhancement-Refactor Refactor on existing functionality. KI-Warning This is a known issue that causes a warning that customers should be aware of
Projects
None yet
Milestone
S111
Development

Successfully merging this pull request may close these issues.
3 participants
@brendankowitz @LTA-Thinking @PTaladay