#2130 MetadataTransfer: single pass over the metadata for wildcard keys by GGraziadei · Pull Request #2133 · apache/stormcrawler

GGraziadei · 2026-09-06T13:42:07Z

MetadataTransfer.filter() runs for every outlink. For each wildcard key (e.g. cookie.*) it called Metadata.keySet(prefix), which streams over all the metadata keys and collects a new Set, then copied each matching key through getValues()/setValues() with key normalisation on both sides.

Change

The configured keys are compiled once into exact keys and wildcard prefixes (cached per set, rebuilt if a subclass modifies the set), and the matching entries are copied in a single pass over the metadata map. Value arrays are shared as before, keys are already normalised.

Micro-benchmark on a 12-key metadata with 3 wildcards and 2 exact keys: ~780 ns to ~265 ns per outlink.

Tests

MetadataTransferTest gains a case checking that wildcard prefixes are matched case-insensitively and selectively (Cookie.* matches cookie.id but not cookies), existing cases unchanged.

For all changes

Is there a issue associated with this PR? Is it referenced in the commit message?
Does your PR title start with #XXXX where XXXX is the issue number you are trying to resolve?
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
Is the code properly formatted with mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false?

For code changes

Have you ensured that the full suite of tests is executed via mvn clean verify?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0? (no new dependencies)
If applicable, have you updated the LICENSE file, including the main LICENSE file? (not applicable)
If applicable, have you updated the NOTICE file, including the main NOTICE file? (not applicable)

rzo1

Currently I am short on time, so first round AI-based review)

The single-pass change itself is right. Metadata.keySet(prefix) builds a stream and collects a new Set per prefix per outlink, and that is worth removing.

I checked the behaviour that is easy to get wrong here, and it is preserved: the old metadata.copy() went through setValues, which drops null and zero-length arrays, and getValues returns null for a zero-length array. The new values.length > 0 / entry.getValue().length == 0 guards match that. Value arrays were shared by reference before too, so the PR description is accurate on that point.

My concern is the caching, not the single pass. See the inline comments on lines 230 and 238.

rzo1 · 2026-09-06T14:11:24Z

+            this.prefixes = prefixList.toArray(new String[0]);
+        }
+
+        private boolean isFor(Set<String> filter) {


return sourceSet == filter && sourceSize == filter.size();

This detects a size change but not a content change of the same size. mdToTransfer is protected final Set<String> with mutable contents, so a subclass doing

mdToTransfer.remove("depth"); mdToTransfer.add("mycustom");

after the first filter() call keeps the size and leaves the compiled filter stale. Every outlink from then on carries the wrong metadata, silently.

The javadoc on line 205 says the cache is "rebuilt if the set has been modified since (e.g. by a subclass)", which is a stronger claim than the code makes good on.

Fixed in a9578fd. CompiledFilter now keeps a HashSet snapshot of the keys it was built from and isFor is snapshot.equals(filter), so a same-size content change (remove("depth"); add("mycustom")) rebuilds the compiled form. Javadoc adjusted to describe what the code does. Test testSameSizeMutationOfTransferSetIsHonoured reproduces the exact scenario and failed on the previous revision.

rzo1 · 2026-09-06T14:11:24Z

+    private volatile CompiledFilter compiledTransfer;
+    private volatile CompiledFilter compiledPersistOnly;
+
+    private CompiledFilter compile(Set<String> filter) {


The two sets are only ever populated in configure(). If that stays true, none of this machinery is needed: compile once at the end of configure() into two final fields and drop the lazy cache, the two volatiles, the identity dispatch and the staleness heuristic.

That keeps the whole measured win, since the per-outlink cost you removed is the stream and Set allocation, not the compile.

If subclass mutation after configure() really has to be supported, the check needs to be sound: either key the cache on a copy of the set contents, or give subclasses an explicit invalidateCompiledFilters() to call.

As it stands the design pays for both options and is correct under neither.

I went with the second option (sound check on a copy of the contents) rather than compiling at the end of configure(), for one reason: configure() is protected and a subclass that does super.configure(conf); mdToTransfer.add("added.*"); is the natural extension point given the two protected sets. Compiling inside the base configure() would leave that subclass with a stale filter. testSubclassCanExtendTransferSetInConfigure pins that case.

The cache is now built lazily on first use and isFor is a Set.equals against the snapshot: a size check plus one hash lookup per key with cached String hashes, no allocation. The per-outlink win (the stream + intermediate Set) is unchanged.

Dropped: the two volatiles (all CompiledFilter fields are final, and the cache is idempotent), the identity dispatch and the null branch. filter(Metadata, Set) is now filter(Metadata, CompiledFilter) fed by transferFilter() / persistOnlyFilter().

rzo1 · 2026-09-06T14:11:24Z

+        CompiledFilter compiled =
+                filter == mdToTransfer
+                        ? compiledTransfer
+                        : filter == mdToPersistOnly ? compiledPersistOnly : null;


filter(Metadata, Set) is private and only ever called with the two fields, so this null branch is unreachable. It disappears if the compile moves into configure().

Gone. The private filter now takes a CompiledFilter directly, obtained from transferFilter() / persistOnlyFilter(), so there is no dispatch on set identity anymore.

rzo1 · 2026-09-06T14:11:24Z

+        if (compiled.prefixes.length > 0) {
+            for (Map.Entry<String, String[]> entry : source.entrySet()) {
+                final String key = entry.getKey();
+                if (entry.getValue().length == 0 || target.containsKey(key)) {


Unguarded dereference of entry.getValue().

Metadata(Map) wraps a caller-supplied map without validating it, so a null value array reaches this line and throws, where the old path called getValues(), got null, and skipped the key. The exact-key branch above is null-checked; this one is not.

Unlikely, but it is one != null away.

Fixed. The wildcard loop reads entry.getValue() once and skips null as well as empty arrays, matching the old getValues() behaviour. testNullValueArrayIsSkipped builds a Metadata over a map with null arrays for both an exact key and a wildcard match; it threw an NPE on the previous revision.

rzo1 · 2026-09-06T14:11:24Z

    static class MyCustomTransferClass extends MetadataTransfer {}
+
+    @Test
+    void testWildcardPrefixIsCaseInsensitiveAndSelective() throws MalformedURLException {


Good addition, and it pins the case-insensitive prefix behaviour.

Nothing covers the caching, which is the part carrying the risk. A test that calls getMetaForOutlink, then mutates mdToTransfer through a subclass without changing its size, then calls it again, would fail today.

Added three tests: testSameSizeMutationOfTransferSetIsHonoured (remove/add keeping the size, second call must see the new keys; failed before), testSubclassCanExtendTransferSetInConfigure (subclass adds a wildcard after super.configure()), and testNullValueArrayIsSkipped. Full core verify: 447 tests, 0 failures.

…ard keys Pre-compile the keys to transfer into exact keys and wildcard prefixes and copy the matching entries in one pass over the metadata, instead of building an intermediate key set per wildcard for every outlink. Micro-benchmark on a 12-key metadata with 3 wildcards: ~780 ns to ~265 ns per outlink. The compiled form is built lazily and keyed on a snapshot of the key set, so a subclass editing mdToTransfer / mdToPersistOnly (in an overridden configure() or later, even without changing the size) is always honoured. Null value arrays in a caller-supplied map are skipped as before. Fixes apache#2130.

rzo1 requested review from dpol1, jnioche and sigee September 6, 2026 13:45

rzo1 assigned GGraziadei Sep 6, 2026

rzo1 added this to the 4.0.0 milestone Sep 6, 2026

rzo1 requested a review from mvolikas September 6, 2026 14:09

rzo1 reviewed Sep 6, 2026

View reviewed changes

GGraziadei force-pushed the perf/metadata-transfer-single-pass branch from 44ff8ab to a9578fd Compare September 6, 2026 22:34

Navigation Menu

Uh oh!

#2130 MetadataTransfer: single pass over the metadata for wildcard keys - #2133

#2130 MetadataTransfer: single pass over the metadata for wildcard keys#2133
GGraziadei wants to merge 1 commit into
apache:mainfrom
GGraziadei:perf/metadata-transfer-single-pass

GGraziadei commented Sep 6, 2026 •

edited

Loading

Uh oh!

rzo1 left a comment

Uh oh!

rzo1 Sep 6, 2026

Uh oh!

GGraziadei Sep 6, 2026

Uh oh!

rzo1 Sep 6, 2026

Uh oh!

GGraziadei Sep 6, 2026

Uh oh!

rzo1 Sep 6, 2026

Uh oh!

GGraziadei Sep 6, 2026

Uh oh!

rzo1 Sep 6, 2026

Uh oh!

GGraziadei Sep 6, 2026

Uh oh!

rzo1 Sep 6, 2026

Uh oh!

GGraziadei Sep 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

GGraziadei commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change

Tests

For all changes

For code changes

Uh oh!

rzo1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

GGraziadei commented Sep 6, 2026 •

edited

Loading