Enhance Metadata Export Subsystem, Part 1 - #12688
Draft
poikilotherm wants to merge 66 commits into
Draft
Conversation
…istryBean #12686 - Moved exporter management logic into a dedicated `ExporterRegistryBean` singleton for improved modularity and maintainability. - Simplified `ExportService` to delegate exporter logic to the new registry.
- Enable injectingthe registry and other components - The export process itself is stateless. State is involved in potential write locks, the loaded plugins, etc. - A stateless coordinator bean scales better for multiple export requests coming in.
…alidator, and storage abstraction #12686 The goal is removing the caching logic from the ExportService. At the same time, a distinct caching subsystem shall have policies about what gets cached, when it expires etc, all independent of a coordinating service like ExportService. This make cognitive loader smaller and allows extension without using more code branches.
…e into FileEmbargoExpiryInvalidator #12686
…eIOCache class #12686 - Reorganized export cache handling into a dedicated `StorageIOCache` service, improving modularity and reducing cognitive load in `ExportService`. - Streamlined caching operations with a unified approach across all storage drivers. - Deprecated legacy unversioned cache keys; introduced versioned aux tag schema for better cache qualification. - Enhanced write atomicity and cache eviction logic. - Remove stale code for size of exports
…OCache #12686 The legacy reading of cached exports is prone to produce bugs in production. When we rely on reading cached exports as prerequisites for other metadata formats, we might end up with stale data. Any export has no knowledge about whether and when an export of another format happened. We keep no provenance per format. Assuming there is a cached "latest" with the legacy file format, it would be read as a prerequisite format, but our invalidation mechanisms would not be able to tell if it's actually stale, because it was not yet re-exported. Any released version is immutable, thus if we rely in lookups on cached objects with the version present in the aux tag, we can be sure we get the latest data.
…constructor #12686 - Added null and blank checks for dataset, version, and formatName to ensure robust usage. - Introduced a convenience constructor for creating cache keys directly from a dataset version and format.
…rvice` package and rename `ExportService` to `ExportServiceBean` #12686 - "ExportServiceBean" is more aligned with the codebase style where EJBs mostly have a "Bean" name suffix. - Also move test classes into the same package (under the test source tree)
- Documented `tryRead`, `deleteQuietly`, and `storageFor` with proper Javadoc. - Clarified the stream-closing intent in `write` to make the leak-avoidance pattern explicit.
…to ExportServiceBean #12686 - Relocated the `invalidators` collection from the sealed interface to the service bean, where it logically belongs as a runtime dependency rather than a static on the contract. - Added a section marker for export data retrieval methods in `ExportServiceBean`. - Noted future plan to replace the static list with a registry pattern once plugins can supply their own invalidation logic.
…tServiceBean 12686 Making it simpler to read inline.
Added `ExportCache` as an CDI (not EJB) injected dependency in the service bean.
- Introduced `clearCachedFormats(DatasetVersion, List<String>)` as the version-specific clearing entry point, with the dataset-level overload delegating via a new `defaultVersion()` helper. - Added `clearCachedFormat(DatasetVersion, String)` to evict a single cache entry by key. - Added `requireExists` and `requireAllExist` validation methods to `ExporterRegistryBean` so format names are checked before eviction.
…12686 Align the related methods into one block, not divided by the cache handling stuff.
…istryBean #12686 - Added `buildFormatRequiredByMap` to build a read-only map of prerequisite format names to the exporters that depend on them. - Added `buildAndVerifyRequirements` to validate registry integrity: all prerequisite formats must have a registered exporter, and no cyclic prerequisite chains may exist. - Integrated the check into initialization as Step 4, failing fast with `ExportException` on any integrity violation (missing prerequisite or cycle).
Added `formatRequiredBy` field to store the prerequisite format dependency map alongside the exporters map, populated during registry initialization. Will be reused during cascaded cache eviction or exporting of formats depending on a certain format.
- Added `buildPrerequisitesChainDepth` to compute the prerequisite chain depth for each format (0 = no prerequisite, N = N levels deep). - Added `buildTopologicalComparator` to create an immutable comparator ordering exporters by depth, with format name as tiebreaker for deterministic results. - Exposed via `getTopologicalComparator()` so callers can sort the exporter list in a dependency-safe order. - Integrated as Step 5 in initialization, stored alongside the existing `formatRequiredBy` map.
…#12686 - Added `SecureTempFiles` utility that creates temp files with `0600` permissions on POSIX systems; on Windows it relies on the per-user `%TEMP%` ACLs. - Replaced raw `Files.createTempFile` in `StorageIOCache.write` with `SecureTempFiles.createOwnerOnlyTempFile` so other local users can no longer read or tamper with export temp files.
The cache key should not be responsible to carry the information about the "where" of an export, just about the "what". Changing dependent methods accordingly. Also, fixed ambiguity with the cache invalidator implementations: the invalidator should look for stale *versions* of dataset, not for the dataset as a whole being stale. The cache is treating versions individually, so they shall get stale individually, too. - Reduced `ExportCacheKey` to a single `auxTag` string, removing `Dataset`/`DatasetVersion` references for thread-safety and GC-friendliness. - Moved `TAG_PREFIX`/`TAG_SUFFIX` into `ExportCacheKey` as public constants. - Added explicit `Dataset` parameter to all `ExportCache` methods (`read`, `write`, `evict`) since the key no longer carries storage context. - Added explicit `DatasetVersion` parameter to `ExportCacheInvalidator.isStale`; updated `FileEmbargoExpiryInvalidator` with null-checks and released/archived status guard. - Updated `StorageIOCache` logging to use `dataset.getId()` instead of the version string.
…ndents set #12686 - Renamed `formatRequiredBy` to `transitiveDependents`, changing the value type from `List<String>` to `Set<String>` to capture all direct and transitive dependents per format. - Replaced `buildPrerequisitesChainDepth` with `buildTransitiveDependents`, which walks each exporter's prerequisite chain and registers it as a dependent of every ancestor format. - Updated `buildTopologicalComparator` to sort by new dependent-set - Merged `buildFormatRequiredByMap` into `verifyRequirements` as the former map is no longer stored for reuse - Moved `getFormatsDependingOn` to `getTransitiveDependents` to reflect the new semantics
…12686 - Introduced sealed `Details` interface exposing `localizedDisplayName`, `formatName`, `mediaType`, `isHarvestable`, and `isAvailableToUsers`, thus avoiding having to retrieve these details from the exporter, saving a roundtrip. - Made `ExporterDetails` record package-private to prevent external instantiation while allowing consumers to read via the interface. - Renamed `getLabels()` to `getDetails()`, returning `List<Details>` with the expanded field set. - Added `get(Details)` lookup method to resolve an exporter by its details object. These can only be created and handed out by the registry, thus we can be sure a matching exporter exists. - Removed unused `Collections` import.
…tCacheKey components #12686 - Replaced the single `auxTag` field with `formatName` and `friendlyVersion` so the key exposes its meaningful parts directly. - Moved `auxTag()` from a static factory into an instance method derived from the record's fields. - Split validation into `checkFormatName` and `checkVersion` private helpers for clearer intent (and compatibility with the constructor needing to be called first thing).
…ServiceBean #12686 These methods (`getExporter`, `isXMLFormat`, `getMediaType`) directly exposed the internal `exporterMap` and are no longer needed now that format details are resolved via the `Details` interface in the registry.
…#12686 Added null check in `get(String formatName)` to return `Optional.empty()` instead of throwing NPE when the underlying Map implementation does not permit null keys.
…ation - Tracks consecutive failures via an `AtomicInteger` streak; escalates from `FINE` to `WARNING` once the streak reaches the configured threshold. - A success resets the streak; a threshold of zero or negative deactivates escalation entirely. - Thread-safe and suitable for sharing across concurrent callers or use in `ConcurrentHashMap` contexts. - Warnings will not be flooding the log once threshold is reached via configurable repeat cycle. - To enable "all clear" messages once the threshold was met, the success recording may then return the number of failures. Using OptionalInt, the logging statement is a one-liner.
…vadoc #12686 - Clarified that the legacy unqualified name is ignored for read/write cycles and only purged via `evictAll`, rather than being a read fallback. - Fix typos
…2686 - Replace static `FINE`-level logging in `tryRead` and `deleteQuietly` with threshold-based escalation via `FailureEscalation` instances (threshold: 256). - Log a recovery warning once consecutive failures drop below the threshold after previously exceeding it. - Include the current failure streak in the read-path log message for operational context.
…EJB #12686 - Introduces a `@Stateless` EJB that funnels all export data production (draft, cached, bulk) through a single path for uniform staleness validation, prerequisite resolution, and error wrapping. - Cached reads consult registered `ExportCacheInvalidator` instances; stale entries are evicted and reported as a miss. - Prerequisite formats are resolved recursively with circular-chain detection via an in-flight `LinkedHashSet`. - Non-cacheable (draft) versions are produced to `SecureTempFiles` with `DELETE_ON_CLOSE` to avoid in-memory retention of large exports. - `IllegalStateException` from exporters is wrapped in `ExportException` with dataset context for consistent reporting across all production paths.
…12686 - Injecting `ExportPipelineBean` as an `@EJB` - Removed the static `invalidators` list and its associated Javadoc from `ExportServiceBean` - they are now owned by the pipeline.
… method #12686 The expansion is used in both eviction and production of formats. Extracting keeps the logic aligned and makes it independently unit-testable.
…at list #12686 Instead of hiding which formats failed in the logs, for admins it's easier to immediately see which formats are affected. Also, make sure the wrapped IOE during the existence check is not suppressed, keeping stack traces available.
- Updated `formatNames` param Javadoc to specify it must not be null (use an empty list to clear all formats). - Added a TODO for purging cache entries and cleaning dangling data post-deaccession.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @Test | ||
| void leavesStorageUntouchedWhenRendererThrowsExportException() { | ||
| ExportException failure = new ExportException("renderer broke"); | ||
| ExportStreamWriter failingWriter = out -> { throw failure; }; |
There was a problem hiding this comment.
🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> reported by reviewdog 🐶
'{' at column 55 should have line break after.
|
|
||
| @Test | ||
| void leavesStorageUntouchedWhenRendererThrowsIOException() { | ||
| ExportStreamWriter failingWriter = out -> { throw new IOException("disk full"); }; |
There was a problem hiding this comment.
🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> reported by reviewdog 🐶
'{' at column 55 should have line break after.
|
coverage: 25.66% (+0.2%) from 25.412% — 12686-enhance-export-subsys into develop |
This comment has been minimized.
This comment has been minimized.
Test Results405 tests ±0 375 ✅ - 12 32m 42s ⏱️ + 7m 16s For more details on these failures, see this check. Results for commit 529702c. ± Comparison against base commit dbc5537. ♻️ This comment has been updated with latest results. |
poikilotherm
force-pushed
the
12686-enhance-export-subsys
branch
from
September 10, 2026 14:41
d6ff80b to
bb36f81
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
…dd short-circuit version guard #12686 The dataset version must reference a valid dataset, otherwise we have no access to the underlying storage. In addition, if the supplied version is not cacheable, don't bother asking the cache to evict something. Note: this relies on the policy in isCacheable(). For now, only drafts are not deemed cacheable. If this is extended to deaccessioned datasets, this will have an impact on this eviction logic!
Using ExportCache in an EJB bean requires WELD (the EJB framework used by Payara) to be able to create proxy implementations. With sealed interfaces, this is not possible. The logs contained this error: Caused by: java.lang.IncompatibleClassChangeError: class edu.harvard.iq.dataverse.export.service.ExportCache$1804761463$Proxy$_$$_WeldClientProxy cannot implement sealed interface edu.harvard.iq.dataverse.export.service.ExportCache
…ons #12686 Use structured logging with `Level.WARNING` and exception objects to enhance debugging.
…onstructor injection #12686 Replace `@Stateless` EJB with `@Dependent` CDI bean, enabling better encapsulation and removing mutable state. Updated `ExportServiceBean` to use field injection as a temporary measure pending full CDI migration. Adjusted tests for constructor-based initialization. The underlying rationale: with the pipeline being an EJB, we had to make it a public type and any method to be called from other EJBs must be public. Otherwise, the EJB proxy will throw, as non-public methods are considers non-business. Yet we don't want anyone outside of the exporter subsystem to interact with the pipeline directly. The only way out: make it a CDI bean.
poikilotherm
force-pushed
the
12686-enhance-export-subsys
branch
from
September 10, 2026 16:56
bb36f81 to
feb3f42
Compare
| void wrapsIllegalStateExceptionFromExporter() { | ||
| // Given | ||
| IllegalStateException cause = new IllegalStateException("field type mismatch"); | ||
| registerExporterMock(BASE, null, (provider, out) -> { throw cause; }); |
There was a problem hiding this comment.
🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> reported by reviewdog 🐶
'{' at column 65 should have line break after.
This comment has been minimized.
This comment has been minimized.
|
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This PR will moves the needle in making the exporting subsystem follow our internal code conventions and good practices.
By splitting the existing "service" into distinctive parts (service, registry, pipeline, invalidators), the architecture is more modular, easier to comprehend and most important: testable.
Which issue(s) this PR closes:
Special notes for your reviewer:
None
Suggestions on how to test this:
There are extensive new tests added, which will be picked up automatically. If you have more suggestions what else should be tested (integration or API test), let me know.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope
Is there a release notes update needed for this change?:
Probably.
Additional documentation:
None so far