Add export history support - #293
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new :exporthistory module that enables durable, checkpointed export of terminal orchestration history to Azure Blob Storage, plus a sample and CI artifact reporting to support validation of the feature.
Changes:
- Introduces the
exporthistoryGradle subproject with entity/orchestrator/activity implementation for BATCH/CONTINUOUS export to Blob Storage. - Adds serializer + escaping utilities and golden parity tests to align exported history format with the .NET implementation (non-entity events).
- Adds a runnable sample and integration/unit tests, plus workflow artifact upload for integration test reports.
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle | Registers the new :exporthistory Gradle module. |
| samples/build.gradle | Adds a runHistoryExportSample task and depends on :exporthistory. |
| samples/src/main/java/io/durabletask/samples/HistoryExportSample.java | Demonstrates scheduling terminal instances and exporting their history to Blob Storage. |
| exporthistory/build.gradle | Defines the new module build (deps, test/integrationTest tasks, publishing/signing, SpotBugs). |
| exporthistory/README.md | Documents install/usage, modes, backend requirements, and format details. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/package-info.java | Package-level docs describing the export history feature components. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportHistoryClientExtensions.java | Provides client-side wiring helpers to create an ExportHistoryClient. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportHistoryWorkerExtensions.java | Registers export entities/orchestrators/activities on a worker builder. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportHistoryClient.java | Client wrapper to create/get/list export jobs backed by entity operations/reads. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportHistoryJobClient.java | Per-job client for create/describe/delete via an operation orchestrator. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportHistoryStorageOptions.java | Configures Blob destination auth + container/prefix/format settings. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJob.java | Durable entity holding job configuration/status/checkpoint/progress and starting the run. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobOrchestrator.java | Orchestrator that pages terminal instances, fans out exports, checkpoints, and continues-as-new. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExecuteExportJobOperationOrchestrator.java | Orchestrator wrapper so clients can await entity operation completion/errors. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ListTerminalInstancesActivity.java | Activity that calls DurableTaskClient.listInstanceIds to page terminal instances. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportInstanceHistoryActivity.java | Activity that validates terminal state, reads history, serializes it, and uploads to Blob. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/BlobExportWriter.java | Azure Blob upload implementation (gzip + content-type/encoding + metadata). |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/HistoryEventSerializer.java | Serializes com.microsoft.durabletask.history events to the export wire format. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/HtmlSafeJsonEscapes.java | Custom Jackson CharacterEscapes to match HTML-safe \\uXXXX escaping rules. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportBlobNaming.java | Deterministic blob naming based on completion timestamp + instanceId (SHA-256). |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobCreationOptions.java | Client-facing creation options and validation for export jobs. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobConfiguration.java | Persisted job configuration used by orchestrator and entity. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobDescription.java | Client-facing projection of entity state. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobTransitions.java | Centralizes valid job-status transition rules + operation name constants. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobStatus.java | Defines job lifecycle states (PENDING/ACTIVE/FAILED/COMPLETED). |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobState.java | Entity state model for job status/config/checkpoint/progress fields. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobRunRequest.java | Orchestrator input referencing the job entity and cycle counter. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobOperationRequest.java | Operation request payload used by the operation orchestrator wrapper. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobQuery.java | Filtering/paging options for listing jobs via entity queries. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobQueryResult.java | Page result wrapper for listing jobs. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportHistoryConstants.java | Defines orchestrator instance-id formatting conventions for export jobs. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobNotFoundException.java | Exception for missing job entity reads. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobInvalidTransitionException.java | Exception for invalid job state transitions. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobClientValidationException.java | Exception for client-side validation/operation failures. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ListTerminalInstancesRequest.java | Activity input model for paging terminal instances. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/InstancePage.java | Activity output model containing instance IDs + next checkpoint. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/CommitCheckpointRequest.java | Entity operation input for committing progress/checkpoints/failures. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportCheckpoint.java | Cursor model used to resume paging across batches. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportFilter.java | Completion-window + terminal-status filter configuration. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportDestination.java | Blob destination (container + prefix) persisted in job config. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportMode.java | Defines BATCH vs CONTINUOUS export modes. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportRequest.java | Per-instance export activity input model. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportResult.java | Per-instance export activity result model (success/blob or error). |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportFailure.java | Captures per-instance failure details for job failure reporting. |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportFormat.java | Defines export kind + schema version (value semantics). |
| exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportFormatKind.java | Enumerates JSON vs JSONL export shapes. |
| exporthistory/src/test/resources/golden/reference-history-events.jsonl | Golden reference output for serializer parity testing. |
| exporthistory/src/test/java/com/microsoft/durabletask/exporthistory/HistoryEventSerializerTest.java | Unit tests for JSON/JSONL shape, null omission, and content-type/extension helpers. |
| exporthistory/src/test/java/com/microsoft/durabletask/exporthistory/HistoryEventSerializerParityTest.java | Byte-for-byte parity tests against golden output and escaping behavior. |
| exporthistory/src/test/java/com/microsoft/durabletask/exporthistory/ExportBlobNamingTest.java | Tests deterministic naming, prefix handling, and timestamp formatting. |
| exporthistory/src/test/java/com/microsoft/durabletask/exporthistory/ExportJobTransitionsTest.java | Tests state transition validity rules. |
| exporthistory/src/test/java/com/microsoft/durabletask/exporthistory/ExportJobOrchestratorTest.java | Regression tests ensuring control-flow exceptions are rethrown by orchestrator. |
| exporthistory/src/test/java/com/microsoft/durabletask/exporthistory/ExportJobDescriptionTest.java | Tests projection correctness and ExportFormat semantics. |
| exporthistory/src/test/java/com/microsoft/durabletask/exporthistory/ExportJobCreationOptionsTest.java | Tests defaults, fluent setters, bounds checks, and mode-specific validation. |
| exporthistory/src/test/java/com/microsoft/durabletask/exporthistory/ExportHistoryIntegrationTest.java | End-to-end integration test against emulator + Azurite to verify blobs written. |
| .github/workflows/build-validation.yml | Uploads exporthistory integration test report artifact for CI visibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Naiyuan Tian (nytian)
left a comment
There was a problem hiding this comment.
Thanks for the PR! Also can we add changelog diff since this is quite breaking change?
3229a39 to
a26b01b
Compare
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
Summary
Impressive, well-architected PR. The durable-execution discipline is the standout: the orchestrator is scrupulously deterministic (ctx.getCurrentInstant(), ctx.createTimer, ctx.allOf, replay-guarded logging) and correctly rethrows OrchestratorBlockedException/ContinueAsNewInterruption — with a dedicated regression test — while the ExportJob entity correctly uses wall-clock Instant.now() (entities aren't replayed). continueAsNew every 5 cycles bounds history; blob names are a deterministic SHA-256 of completedTimestamp|instanceId so retries are idempotent; the serializer reproduces the .NET wire format meticulously and pins it with a golden-file test. Clean module/build wiring, pinned deps, correct Java 8/11 split, strong unit coverage, and a real emulator+Azurite E2E test.
The findings below are mostly robustness/efficiency and test-coverage gaps — none are determinism bugs.
Findings (by severity)
Medium
1. The per-activity RetryPolicy never fires (dead retry). ExportInstanceHistoryActivity.run() catches all exceptions and returns ExportResult.failure(...) instead of throwing. The framework therefore sees every invocation as a success, so EXPORT_ACTIVITY_RETRY_OPTIONS (3 attempts, exp backoff) attached at the call site in ExportJobOrchestrator.exportBatch is never triggered. Transient faults (blob 503, gRPC hiccup while reading history) get no fine-grained retry — they escalate straight to the coarse batch-level retry. Either let the activity throw on transient failures (so the RetryPolicy applies) or remove the unused policy so it doesn't imply per-instance retries that don't happen.
2. Batch retry re-exports already-succeeded instances. processBatchWithRetry re-runs exportBatch over the full instance-id list on every attempt, not just the failed subset. On any partial failure, previously-successful instances are re-read (full history) and re-uploaded (up to 3×). Idempotent blob names mean no corruption, but it's wasted history reads + blob writes. Recommend retrying only the failed subset. (Compounds with #1: the batch retry is the only retry, and it's coarse.)
3. All-or-nothing batch failure discards progress; one poison instance fails the whole job. If any instance in a page fails after retries, the orchestrator commits no cursor advance and transitions the job to FAILED — throwing away the (up to maxInstancesPerBatch−1) successful exports' progress, and letting a single unexportable instance kill the entire export job. Consider quarantining/skipping poison instances and advancing the cursor past them (recording per-instance failures) rather than failing the whole job.
4. Non-atomic 3-call blob upload. BlobExportWriter.upload issues upload(), then setHttpHeaders(), then setMetadata() as three separate REST calls. If setHttpHeaders fails after upload succeeds, a gzipped blob is left without Content-Encoding: gzip → consumers won't auto-decompress and will read gzip bytes as text. Set headers + metadata in the single upload (uploadWithResponse(new BlobParallelUploadOptions(data).setHeaders(...).setMetadata(...), ...)). Also createIfNotExists() runs on every instance upload — an extra REST round-trip per instance under high fan-out (consider ensuring the container once per job/worker).
Low–Medium
5. Builder overload leaks a DurableTaskClient. ExportHistoryClientExtensions.useExportHistory(DurableTaskGrpcClientBuilder, storage) calls builder.build() to create a DurableTaskClient (AutoCloseable, owns a gRPC channel), but ExportHistoryClient is not AutoCloseable and never exposes/closes it → the caller has no way to close the channel. Either make ExportHistoryClient own and close it (implement AutoCloseable), or drop this overload in favor of the existing-client one (where the caller owns the lifecycle, as the sample does).
Low
6. Pagination empty-token fragility (inherited from #292). ListTerminalInstancesActivity nicely handles a null continuation token by advancing to the last instance ID (preventing a restart), but if #292's listInstanceIds ever returns a present-but-empty "" token (the edge case flagged on #292), that "" would be committed as the checkpoint cursor and could restart BATCH paging from the beginning → duplicate exports / non-termination. Treat empty-string tokens as null here too, independent of the #292 fix.
7. Entity-event serialization path is almost untested. Core events have excellent byte-for-byte golden coverage, but the reflective writeEntity path (7 entity event types, explicitly "non-parity by design") is exercised for only 1 type (EntityLockGranted). The other 6 (EntityOperationCalled/Completed/Failed/Signaled, EntityLockRequested, EntityUnlockSent) have no serialization test — and that path relies on jackson-datatype-jsr310 being auto-registered (findAndAddModules) to render Instant. Add at least one assertion pinning the reflective entity shape.
8. Missing CONTINUOUS-mode and retry/backoff tests. The integration test covers BATCH e2e only. The CONTINUOUS idle-loop/resume/delete-while-active path and the retry/backoff logic have no coverage.
9. taskScheduleId vs taskScheduledId reads as a typo. In HistoryEventSerializer.parentInstanceMap the key is "taskScheduleId" (no 'd'); everywhere else it's "taskScheduledId". This appears to match .NET's ParentInstance.TaskScheduleId quirk (intentional parity), but a future maintainer will likely "fix" it and silently break parity. Add a comment marking it deliberate, and ideally a golden line covering a parent-instance to pin it.
10. CHANGELOG not updated. New public module + API surface, but no CHANGELOG.md entry; the PR checklist item is unchecked (same as #292).
Nits
processBatchWithRetrycomputesexportedCounton the failure path, but the failure commit passes0, 0— the value is discarded.ExportHistoryClient.listJobsfilters client-side after server-side paging, so a returned page can be smaller thanpageSize(or empty) even when more matches exist on later pages;createdFrom/createdToare exclusive on both ends — worth documenting.AbstractTaskEntitynow callssetAccessible(true)for any non-public entity declaring class — reasonable, contained, and covered by a newTaskEntityTestcase; just noting it broadens reflective access for all entities, not onlyExportJob.ExecuteExportJobOperationOrchestratorround-trips the operation payload asObject(→ generic map → re-serialized to the entity's concrete type). It works (the E2E proves it), but it's a latent serialization coupling worth a comment.
Questions for the author
- For a batch where 1 of N instances is permanently unexportable, is failing the whole job (and discarding the batch's progress) the intended behavior, or should poison instances be skipped?
- Was the activity meant to surface transient errors as thrown failures so
EXPORT_ACTIVITY_RETRY_OPTIONSengages? Today it swallows them and the policy is inert.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 57 changed files in this pull request and generated no new comments.
Suppressed comments (3)
samples/src/main/java/io/durabletask/samples/HistoryExportSample.java:140
- Log output is missing a space after the colon, which makes the sample’s results formatting inconsistent with the other lines (and harder to read).
exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/HistoryEventSerializer.java:309 tagsMappreserves the iteration order of the sourcetagsmap. In the public history model, tags are copied into aHashMap(e.g., TaskScheduledEvent ctor), whose iteration order is not guaranteed. That can make exported JSON non-deterministic (breaking byte-for-byte parity/idempotence). Consider sorting tag keys during serialization.
exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobOrchestrator.java:71CONTINUE_AS_NEW_FREQUENCYis 5, but the current>check causes continue-as-new to happen on the 6th cycle (processedCycles: 1..6). This off-by-one makes orchestration history larger than intended.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 61 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
samples/src/main/java/io/durabletask/samples/HistoryExportSample.java:115
- This file imports OrchestrationRuntimeStatus but doesn't reference it, which is a Java compilation error for unused imports. Either remove the import or use the type explicitly (e.g., by storing md.getRuntimeStatus() in a variable).
exporthistory/src/test/java/com/microsoft/durabletask/exporthistory/ExportHistoryWorkerExtensionsTest.java:40 - TaskEntity is imported but never used in this test, which is a Java compilation error for unused imports. Either remove the import or bind factory.create() to a TaskEntity variable before asserting its type.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 61 changed files in this pull request and generated no new comments.
Suppressed comments (2)
samples/src/main/java/io/durabletask/samples/HistoryExportSample.java:140
- Minor output formatting: this log line is missing a space after the colon, which makes the sample output inconsistent with the surrounding lines.
exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/BlobExportWriter.java:107 - BlobExportWriter buffers the full serialized history twice in memory (String -> UTF-8 byte[], then optionally gzip -> another byte[]). Large orchestration histories can be very large, so this can create high memory pressure or OOMs during export. Consider streaming compression + upload (e.g., gzip an InputStream/Flux directly into the upload) so memory usage is bounded.
byte[] contentBytes = content.getBytes(StandardCharsets.UTF_8);
boolean gzip = HistoryEventSerializer.isCompressed(format);
byte[] payload = gzip ? gzip(contentBytes) : contentBytes;
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
Reviewed the full diff against main (#292 is merged, so the diff is clean). Overall this is well-structured, thoroughly documented code with genuinely good parity testing. Findings are ordered by severity.
Note on method: this was a static review — I did not run the build or tests, so the correctness findings below are derived from tracing code paths rather than from observed failures.
🔴 Blocking
1. Permanent-failure path throws ExportJobInvalidTransitionException, masking the real error
ExportJobOrchestrator.java:116-120 → ExportJob.java:155-163 → ExportJobTransitions.java:55-56
// ExportJobOrchestrator
commitCheckpoint(ctx, jobEntityId, 0, 0, null, batchResult.failures); // entity sets status = FAILED
throw new IllegalStateException("... batch export failed after 3 retry attempts ...");
// → caught at L129 → markAsFailed(...) → isValidTransition("MarkAsFailed", FAILED, FAILED) == false → throwscommitCheckpoint already transitions the job to FAILED when checkpoint == null && !failures.isEmpty(). The outer catch then calls markAsFailed, whose transition rule requires from == ACTIVE. The entity operation faults, and because that exception is thrown from inside the catch block it is not re-caught — it propagates out and replaces ex. The orchestration terminates with a confusing "invalid transition" message instead of the actual batch-failure reason.
(lastError on the job is still written by commitCheckpoint, so the state isn't wrong — but the orchestration's terminal error is misleading, and the entity throws an exception that should never have happened.)
This is invisible to the current tests: ExportJobOrchestratorTest.run_batchExportFailure_exhaustsRetriesThenFaultsAndMarksJobFailed mocks callEntity, so the real ExportJob entity never executes and the transition check never runs.
Suggested fix: either drop the status mutation from commitCheckpoint and let markAsFailed own the transition, or make MarkAsFailed idempotent (from == ACTIVE || from == FAILED). Either way, please add an entity-level test that calls commitCheckpoint(with failures) followed by markAsFailed(...) without mocks.
2. Re-created jobs reuse a fixed orchestrator instance ID, and the failure is silent
ExportHistoryConstants.java:26 · ExportJob.java:118-131 · ExportJobTransitions.java:48-52
Create is explicitly allowed from COMPLETED and FAILED, but the orchestrator instance ID is deterministic ("ExportJob-" + jobId) and only delete() purges the previous generation. So calling createJob("nightly") a second time re-signals Run, which starts an orchestration with an ID that already exists.
Compounding this: TaskEntityContext.startNewOrchestration only queues a PendingAction (TaskEntityExecutor.java:269-281) — it is fire-and-forget and cannot throw synchronously. That makes the try/catch (RuntimeException) at ExportJob.java:125-131 unreachable, so it provides false confidence. If the backend rejects the start, the job stays ACTIVE forever with no orchestrator running and nothing surfaced to the caller.
Suggested fix: include a generation/run counter in the orchestrator instance ID, and remove the misleading catch (or replace it with something that can actually observe the failure).
3. The activity RetryPolicy is dead code, and orchestrator retries re-export the whole page
ExportInstanceHistoryActivity.java:76-79 · ExportJobOrchestrator.java:40-43, 150-182
The activity catches Exception and returns ExportResult.failure(...), so the task always succeeds from the orchestrator's point of view. EXPORT_ACTIVITY_RETRY_OPTIONS (3 attempts, 15s→60s backoff) can therefore never fire.
That leaves only the orchestrator-level retry — and processBatchWithRetry re-runs exportBatch(ctx, instanceIds, config) with the original full list, so every already-successful instance is re-fetched and re-uploaded on each of the 3 attempts. Blob writes are idempotent (fixed hash name, overwrite), so the result is correct, but it costs up to 3× the history reads and blob PUTs for a batch with a single poison instance.
Suggested fix: retry only the failed subset; and either let the activity throw so the declared policy becomes meaningful, or drop the unused TaskOptions.
🟡 Medium
4. Stale vs. fresh config mixed in the same loop. config is captured once (ExportJobOrchestrator.java:63), but currentState.getConfig() is re-read every iteration (L80, L89). Mode, destination, format and maxParallelExports come from the stale copy while filter and batch size come from the fresh one. Worth picking one consistently.
5. The whole history is buffered in memory ~3–4×. HistoryEventSerializer.serialize builds a StringBuilder → String → UTF-8 byte[] → gzip byte[], with up to maxParallelExports = 32 activities running concurrently on a worker. The PR description says it "streams getOrchestrationHistory", but nothing is actually streamed. This looks like a real OOM risk for large histories — at minimum worth documenting the size limit.
6. maxParallelExports is unreachable from the public API. ExportJobConfiguration has the field and setter (default 32), but ExportJobCreationOptions exposes no way to set it and the 5-arg constructor doesn't pass it through.
7. ExportJobRunRequest.processedCycles is always 0. Both call sites (ExportJob.java:121 and ExportJobOrchestrator.java:69) pass 0, so the field never carries state across continueAsNew. Either use it or remove it.
🟢 Low / polish
DurableTaskClient.java:250-251— unintended whitespace regression. The javadoc indentation ofgetOrchestrationHistorywas changed (/**→/**). Looks unrelated to this PR; please revert.AbstractTaskEntity.makeDeclaringClassAccessible—setAccessible(true)can throwInaccessibleObjectExceptionunder JPMS on Java 16+. It's also invoked on every dispatch rather than once during method lookup/caching.HtmlSafeJsonEscapesis allocated per event (HistoryEventSerializer.java:389), andgetEscapeSequencecallsString.formatfor every escaped character. This is a hot path for non-ASCII payloads — a static singleton with a precomputed escape table would be much cheaper.HtmlSafeJsonEscapes.getEscapeCodesForAscii()returns the internal array (classicEI_EXPOSE_REP).BlobExportWriter.upload:100callscreateIfNotExists()on every single blob — one extra REST round-trip per exported instance.ExportHistoryJobClient.create():71mutates the caller'soptionsobject viasetDestination.HistoryEventSerializer.writeValue:426-428stringifies unknown numeric types — aDouble/BigDecimalfield would silently serialize as a JSON string. Safe today given whatcoreMapputs in, but a latent parity hazard.- Two NPE paths get swallowed into opaque failures:
statusString(null)(L313) andmetadata.getLastUpdatedAt() == null→ NPE informatTimestamp. Both are caught by the activity's catch-all and surface asExportResult.failure(id, null)→ "Unknown error". - Test coverage gap:
ExportJobTestcovers onlycreate. There are no entity-level tests forcommitCheckpoint,markAsCompleted,markAsFailed, orrun— which is exactly why finding #1 slipped through. - PR checklist: the CHANGELOG was updated, but the checklist boxes for docs and CHANGELOG are still unchecked.
✅ Things done well
HistoryEventSerializerParityTestpinned against a golden.jsonlcaptured fromMicrosoft.Azure.DurableTask.Coreis the right way to lock a wire format, and the deliberate-quirk comments (thetaskScheduleIdmisspelling,TimerFired eventId:-1) are exactly what stops a future "helpful" fix from silently breaking compatibility.ExportJobOrchestratorTestexplicitly regression-guards theOrchestratorBlockedException/ContinueAsNewInterruptionrethrow — a subtle and very easy-to-break invariant.ListTerminalInstancesActivity.java:51-59correctly handles the null-continuation-token-on-non-empty-page case. I checkedDurableTaskGrpcClient.listInstanceIdsto confirm the token really is alastInstanceKeycursor, so the fallback is sound.- Single atomic
uploadWithResponse(headers + metadata + content together) avoids ever leaving a gzipped blob without itsContent-Encoding. - The
AbstractTaskEntityaccessibility fix ships with a targeted regression test (reflectionDispatch_publicOperationOnPrivateEntityClass).
Nice work overall — findings #1–#3 are the ones I'd want resolved before merge, since they're in the failure and re-create paths that the current mock-heavy tests can't catch. Everything else is fine as follow-up.
|
wangbill (@YunchuWang) , thanks for the thorough review! I've addressed all the items that can be fixed on the Java side without diverging from the .NET SDK. The failure/re-create-path findings (#1–#4, #6, #7) are faithful ports of identical behavior in 🔴 Blocking
🟡 Medium
🟢 Low / polish
All parity-safe changes build clean and pass the existing suite. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 60 changed files in this pull request and generated no new comments.
Suppressed comments (2)
exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJob.java:90
- ExportJob.create(..) resets most state fields but leaves orchestratorInstanceId untouched. When recreating a job (allowed from FAILED/COMPLETED) or if Run fails before setting a new ID, describe() can report a stale orchestrator instance ID from a prior run.
this.state.setScannedInstances(0);
this.state.setExportedInstances(0);
this.state.setCheckpoint(null);
this.state.setLastCheckpointTime(null);
exporthistory/src/main/java/com/microsoft/durabletask/exporthistory/ExportJobOrchestrator.java:71
- The continueAsNew guard is off by one: processedCycles is incremented before the check, but the condition uses '>'. With CONTINUE_AS_NEW_FREQUENCY=5 this runs 6 cycles per generation, which undermines the intent to bound orchestration history.
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
Thanks for addressing the Java-side feedback. Varshitha Bachu (@bachuv), please cut and link a follow-up issue for the cross-SDK fix to the permanent-failure path: commitCheckpoint(failures) transitions the job to FAILED, after which markAsFailed attempts the invalid FAILED -> FAILED transition and may mask the original export error. Please include an entity-level regression test, rather than only the mocked orchestrator test, so the real transition path is covered.
Approving this PR with that follow-up tracked separately.
Issue describing the changes in this PR
Adds the
:exporthistorymodule — durable, checkpointed export of terminal orchestration history to Azure Blob Storage. Stacked on #292.Architecture
ExportJob(entity) — single source of truth: config, status, checkpoint cursor, progress counters. Owns the state machine (PENDING → RUNNING → COMPLETED/FAILED); signals a run on create.ExportJobOrchestrator— pages terminal instances viaListTerminalInstancesActivity, fans outExportInstanceHistoryActivityper instance, commits checkpoints back to the entity, handlesBATCHvsCONTINUOUS, retries with bounded backoff, andcontinueAsNews periodically to keep history bounded.ExecuteExportJobOperationOrchestratorwraps entity operations.ExportInstanceHistoryActivity— reads instance metadata (terminal check + completion timestamp), streamsgetOrchestrationHistory, serializes, and uploads viaBlobExportWriter.ExportHistoryClient/ExportHistoryJobClient+useExportHistoryworker/client extensions.Serialization (byte-for-byte .NET parity)
HistoryEventSerializer+HtmlSafeJsonEscapesreproduce the .NET export wire format exactly (field order,eventType/isPlayed, PascalCase enums, HTML-safe\uXXXXescaping). Pinned byHistoryEventSerializerParityTestagainst golden captured fromMicrosoft.Azure.DurableTask.Core(reference-history-events.jsonl).TimerFired eventId:-1,HistoryStatedefaults0001-01-01/size:0,ContinueAsNewas anExecutionCompleted.eventTypediscriminator (non-parity by design).Testing
@Tag("integration")): end-to-endBATCHexport against the DTS emulator + Azurite — verifies job reachesCOMPLETEDand blobs are written. Wired intobuild-validation.yml.Pull request checklist
CHANGELOG.md