Skip to content

Users/sergal/reindex workflow - #5711

Open
SergeyGaluzo wants to merge 76 commits into
mainfrom
users/sergal/reindex-workflow
Open

Users/sergal/reindex workflow#5711
SergeyGaluzo wants to merge 76 commits into
mainfrom
users/sergal/reindex-workflow

Conversation

@SergeyGaluzo

Copy link
Copy Markdown
Contributor

No description provided.

@SergeyGaluzo
SergeyGaluzo requested a review from a team as a code owner August 3, 2026 01:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the reindex workflow across SQL Server and Cosmos DB paths, introducing a SQL-optimized “surrogate id range” execution flow, new job polling configuration, and updated reindex job/definition payload shapes (including per-search-parameter status metadata).

Changes:

  • Add SQL reindex execution path based on surrogate-id subranges (SearchBySurrogateIdRange) and adjust orchestrator/processing jobs accordingly.
  • Update reindex job record/progress reporting and reindex payload schema (notably SearchParameterUrlStatuses with a JSON converter).
  • Add new reindex operational configuration knobs (polling interval, batch size) and propagate them through test and deployment pipelines.

Reviewed changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tools/Microsoft.Health.Fhir.R4.ResourceParser/Code/MinimalSearchParameterDefinitionManager.cs Adds new interface member (currently stubbed).
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerSearchParameterStatusDataStoreTests.cs Updates ReindexJobRecord construction.
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTests.cs Updates conflict expectations for reindex writes.
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Search/SqlServerSearchServiceIntegrationTests.cs Removes legacy reindex-search tests; uses surrogate range API.
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Operations/Reindex/ReindexSearchTests.cs Skips hash-based tests for SQL; updates query params.
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Operations/Reindex/ReindexJobTests.cs Updates job hosting setup and job result assertions.
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Operations/FhirOperationDataStoreReindexTests.cs Adjusts StartTime and ReindexJobRecord construction.
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/FailingSearchParameterStatusManager.cs Adds TryLogEvent stub to match interface.
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/InProcTestFhirServer.cs Sets reindex polling interval for tests.
src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs Avoid throw on version conflicts; log warning instead.
src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs Adds surrogate range API; refactors export time-travel hint handling.
src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs Changes when query-hints are built (StartSurrogateId-based).
src/Microsoft.Health.Fhir.Shared.Core/Extensions/ReindexJobRecordExtensions.cs Reworks progress calculation using succeeded vs failed counts.
src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Search/SearchServiceTests.cs Adjusts override visibility for refactored virtual method.
src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Operations/Reindex/ReindexProcessingJobTests.cs Updates tests to SQL surrogate-range and new payload schema.
src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Operations/Reindex/ReindexOrchestratorJobTests.cs Updates orchestrator tests for SQL/Cosmos branching and payload schema.
src/Microsoft.Health.Fhir.Shared.Api.UnitTests/Controllers/ReindexControllerTests.cs Adds stronger assertions on returned reindex Parameters payload.
src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/CosmosFhirDataStore.cs Treats version conflicts as warnings; returns wrapper instead of throwing.
src/Microsoft.Health.Fhir.CosmosDb/Features/Search/FhirCosmosSearchService.cs Updates override visibility for refactored virtual method.
src/Microsoft.Health.Fhir.Core/Features/Search/SearchService.cs Makes reindex/search helpers virtual; adds surrogate-range entry point.
src/Microsoft.Health.Fhir.Core/Features/Search/Registry/ISearchParameterStatusManager.cs Adds TryLogEvent to status manager contract.
src/Microsoft.Health.Fhir.Core/Features/Search/ISearchService.cs Adds SearchBySurrogateIdRange to search service contract.
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexProcessingJobResult.cs Removes SearchParameterUrls from processing-job result payload.
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexProcessingJobDefinition.cs Introduces SearchParameterUrlStatuses with converter.
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexProcessingJob.cs Refactors processing execution into SQL subranges vs Cosmos continuation tokens.
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexOrchestratorJob.cs Refactors orchestrator scheduling/polling and status-update flow.
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/Models/ReindexSearchParameterUrlStatusesConverter.cs New JSON converter for tuple-based URL/status list.
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/Models/ReindexJobRecord.cs Simplifies constructor, changes defaults, updates max write limit.
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/Models/ReindexJobQueryResourceCountsConverter.cs Removes legacy converter.
src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/CreateReindexRequestHandler.cs Updates handler to new ReindexJobRecord ctor shape.
src/Microsoft.Health.Fhir.Core/Features/Operations/FhirOperationDataStoreBase.cs Reworks how reindex record fields are derived from queue jobs.
src/Microsoft.Health.Fhir.Core/Features/Definition/SupportedSearchParameterDefinitionManager.cs Adds new interface member (currently stubbed).
src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs Makes derived-type expansion API public for interface.
src/Microsoft.Health.Fhir.Core/Features/Definition/SearchableSearchParameterDefinitionManager.cs Adds new interface member (currently stubbed).
src/Microsoft.Health.Fhir.Core/Features/Definition/ISearchParameterDefinitionManager.cs Adds GetDerivedResourceTypes to contract.
src/Microsoft.Health.Fhir.Core/Configs/ReindexJobConfiguration.cs Adds polling interval + batch size config knobs; removes legacy knobs.
src/Microsoft.Health.Fhir.Core.UnitTests/Features/Operations/Reindex/ReindexHandlerTests.cs Updates ReindexJobRecord construction.
build/pr-variables.yml Adds pipeline variable for reindex polling interval.
build/jobs/scripts/Provision-AcaDeploy.ps1 Wires reindex polling interval into ACA env settings.
build/jobs/provision-deploy.yml Passes new polling variable to deploy script.
build/ci-variables.yml Adds CI variable for reindex polling interval.
Suppressed comments (1)

src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexOrchestratorJob.cs:159

  • CreatedJobs is set from currentJobs.Count, but currentJobs is only populated for Cosmos and is never refreshed after enqueuing new jobs. This will report 0 created jobs for fresh Cosmos runs and for all SQL runs (where group jobs aren’t fetched).
                if (_isSql || !currentJobs.Any())
                {
                    await CreateReindexProcessingJobsAsync();
                }
                else // cosmos job restart

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexProcessingJobDefinition.cs:35

  • SearchParameterUrlStatuses can be null when deserializing older processing job payloads (property absent) or when callers forget to populate it. The orchestrator and operation data store iterate this collection without null checks, so a null value will cause runtime failures. Initializing it to an empty collection makes deserialization and usage safer.
        [JsonConverter(typeof(ReindexSearchParameterUrlStatusesConverter))]
        public IReadOnlyCollection<(string Url, SearchParameterStatus Status)> SearchParameterUrlStatuses { get; set; }

src/Microsoft.Health.Fhir.Core/Features/Operations/Reindex/ReindexOrchestratorJob.cs:158

  • On the SQL path, currentJobs is intentionally left empty, so setting _result.CreatedJobs = currentJobs.Count will always report 0 jobs created even when CreateReindexProcessingJobsAsync enqueues work. Also on the Cosmos restart path, job.Def.SearchParameterUrlStatuses can be null when deserializing legacy processing job definitions (property missing), which will cause a NullReferenceException inside PopulateProcessingLookups.
                if (_isSql || !currentJobs.Any())
                {
                    await CreateReindexProcessingJobsAsync();
                }
                else // cosmos job restart

src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs:109

  • SearchOptionsFactory sets QueryHints when only _startSurrogateId is present. Downstream, SqlServerSearchService.PopulateSqlCommandFromQueryHints assumes the hints also include _type and _endSurrogateId (it uses First(...)), so requests that include only _startSurrogateId will throw InvalidOperationException at runtime. Only enabling QueryHints when the full required surrogate-id range is present avoids this.
    src/Microsoft.Health.Fhir.Core/Features/Operations/FhirOperationDataStoreBase.cs:462
  • PopulateReindexJobRecordDataFromJobs assumes SearchParameterUrlStatuses is always present in the child job definition. If a reindex deployment restarts with legacy child jobs still in the queue (which used SearchParameterUrls), SearchParameterUrlStatuses will be null and this line will throw, breaking GetReindexJobById/ListReindexJobs.
            var urls = JsonConvert.DeserializeObject<ReindexProcessingJobDefinition>(subJob.Definition).SearchParameterUrlStatuses.Select(x => x.Url).ToList();
            foreach (var url in urls)

test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Operations/Reindex/ReindexSearchTests.cs:52

  • Typo in comment: "supprted ony" -> "supported only".
    src/Microsoft.Health.Fhir.Core/Configs/ReindexJobConfiguration.cs:48
  • Typo in comment: "orchetrator" -> "orchestrator".
        /// Controls orchetrator job info polling interval
        /// </summary>

@SergeyGaluzo

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

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.

4 participants