rzo1 commented on code in PR #2133:
URL: https://github.com/apache/stormcrawler/pull/2133#discussion_r3944191288
##########
core/src/main/java/org/apache/stormcrawler/util/MetadataTransfer.java:
##########
@@ -164,19 +168,86 @@ public Metadata filter(Metadata metadata) {
* with the prefix will be added.
*/
private Metadata filter(Metadata metadata, Set<String> filter) {
- Metadata filteredMetadata = new Metadata();
+ final CompiledFilter compiled = compile(filter);
+ final Map<String, String[]> source = metadata.asMap();
+ final Map<String, String[]> target = new HashMap<>();
+
+ // exact keys: direct lookups
+ for (String key : compiled.exactKeys) {
+ final String[] values = source.get(key);
+ if (values != null && values.length > 0) {
+ target.put(key, values);
+ }
+ }
- for (String key : filter) {
- if (key.endsWith("*")) {
- String prefix = key.substring(0, key.length() - 1);
- for (String k : metadata.keySet(prefix)) {
- metadata.copy(filteredMetadata, k);
+ // wildcards: a single pass over the metadata for all the prefixes,
+ // without allocating an intermediate key set per prefix
+ 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)) {
+ continue;
+ }
+ for (String prefix : compiled.prefixes) {
+ if (key.startsWith(prefix)) {
+ target.put(key, entry.getValue());
+ break;
+ }
}
- } else {
- metadata.copy(filteredMetadata, key);
}
}
- return filteredMetadata;
+ return new Metadata(target);
+ }
+
+ /**
+ * Pre-computed, normalised form of a set of keys to transfer: exact keys
and wildcard prefixes.
+ * Cached per set and rebuilt if the set has been modified since (e.g. by
a subclass).
+ */
+ private static final class CompiledFilter {
+ private final Set<String> sourceSet;
+ private final int sourceSize;
+ private final Set<String> exactKeys;
+ private final String[] prefixes;
+
+ private CompiledFilter(Set<String> filter) {
+ this.sourceSet = filter;
+ this.sourceSize = filter.size();
+ final Set<String> exact = new HashSet<>();
+ final List<String> prefixList = new ArrayList<>();
+ for (String key : filter) {
+ final String normalised = key.toLowerCase(Locale.ROOT);
+ if (normalised.endsWith("*")) {
+ prefixList.add(normalised.substring(0, normalised.length()
- 1));
+ } else {
+ exact.add(normalised);
+ }
+ }
+ this.exactKeys = exact;
+ this.prefixes = prefixList.toArray(new String[0]);
+ }
+
+ private boolean isFor(Set<String> filter) {
+ return sourceSet == filter && sourceSize == filter.size();
+ }
+ }
+
+ private volatile CompiledFilter compiledTransfer;
+ private volatile CompiledFilter compiledPersistOnly;
+
+ private CompiledFilter compile(Set<String> filter) {
Review Comment:
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 `volatile`s, 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.
##########
core/src/main/java/org/apache/stormcrawler/util/MetadataTransfer.java:
##########
@@ -164,19 +168,86 @@ public Metadata filter(Metadata metadata) {
* with the prefix will be added.
*/
private Metadata filter(Metadata metadata, Set<String> filter) {
- Metadata filteredMetadata = new Metadata();
+ final CompiledFilter compiled = compile(filter);
+ final Map<String, String[]> source = metadata.asMap();
+ final Map<String, String[]> target = new HashMap<>();
+
+ // exact keys: direct lookups
+ for (String key : compiled.exactKeys) {
+ final String[] values = source.get(key);
+ if (values != null && values.length > 0) {
+ target.put(key, values);
+ }
+ }
- for (String key : filter) {
- if (key.endsWith("*")) {
- String prefix = key.substring(0, key.length() - 1);
- for (String k : metadata.keySet(prefix)) {
- metadata.copy(filteredMetadata, k);
+ // wildcards: a single pass over the metadata for all the prefixes,
+ // without allocating an intermediate key set per prefix
+ 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)) {
+ continue;
+ }
+ for (String prefix : compiled.prefixes) {
+ if (key.startsWith(prefix)) {
+ target.put(key, entry.getValue());
+ break;
+ }
}
- } else {
- metadata.copy(filteredMetadata, key);
}
}
- return filteredMetadata;
+ return new Metadata(target);
+ }
+
+ /**
+ * Pre-computed, normalised form of a set of keys to transfer: exact keys
and wildcard prefixes.
+ * Cached per set and rebuilt if the set has been modified since (e.g. by
a subclass).
+ */
+ private static final class CompiledFilter {
+ private final Set<String> sourceSet;
+ private final int sourceSize;
+ private final Set<String> exactKeys;
+ private final String[] prefixes;
+
+ private CompiledFilter(Set<String> filter) {
+ this.sourceSet = filter;
+ this.sourceSize = filter.size();
+ final Set<String> exact = new HashSet<>();
+ final List<String> prefixList = new ArrayList<>();
+ for (String key : filter) {
+ final String normalised = key.toLowerCase(Locale.ROOT);
+ if (normalised.endsWith("*")) {
+ prefixList.add(normalised.substring(0, normalised.length()
- 1));
+ } else {
+ exact.add(normalised);
+ }
+ }
+ this.exactKeys = exact;
+ this.prefixes = prefixList.toArray(new String[0]);
+ }
+
+ private boolean isFor(Set<String> filter) {
+ return sourceSet == filter && sourceSize == filter.size();
+ }
+ }
+
+ private volatile CompiledFilter compiledTransfer;
+ private volatile CompiledFilter compiledPersistOnly;
+
+ private CompiledFilter compile(Set<String> filter) {
+ CompiledFilter compiled =
+ filter == mdToTransfer
+ ? compiledTransfer
+ : filter == mdToPersistOnly ? compiledPersistOnly :
null;
Review Comment:
`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()`.
##########
core/src/main/java/org/apache/stormcrawler/util/MetadataTransfer.java:
##########
@@ -164,19 +168,86 @@ public Metadata filter(Metadata metadata) {
* with the prefix will be added.
*/
private Metadata filter(Metadata metadata, Set<String> filter) {
- Metadata filteredMetadata = new Metadata();
+ final CompiledFilter compiled = compile(filter);
+ final Map<String, String[]> source = metadata.asMap();
+ final Map<String, String[]> target = new HashMap<>();
+
+ // exact keys: direct lookups
+ for (String key : compiled.exactKeys) {
+ final String[] values = source.get(key);
+ if (values != null && values.length > 0) {
+ target.put(key, values);
+ }
+ }
- for (String key : filter) {
- if (key.endsWith("*")) {
- String prefix = key.substring(0, key.length() - 1);
- for (String k : metadata.keySet(prefix)) {
- metadata.copy(filteredMetadata, k);
+ // wildcards: a single pass over the metadata for all the prefixes,
+ // without allocating an intermediate key set per prefix
+ 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)) {
+ continue;
+ }
+ for (String prefix : compiled.prefixes) {
+ if (key.startsWith(prefix)) {
+ target.put(key, entry.getValue());
+ break;
+ }
}
- } else {
- metadata.copy(filteredMetadata, key);
}
}
- return filteredMetadata;
+ return new Metadata(target);
+ }
+
+ /**
+ * Pre-computed, normalised form of a set of keys to transfer: exact keys
and wildcard prefixes.
+ * Cached per set and rebuilt if the set has been modified since (e.g. by
a subclass).
+ */
+ private static final class CompiledFilter {
+ private final Set<String> sourceSet;
+ private final int sourceSize;
+ private final Set<String> exactKeys;
+ private final String[] prefixes;
+
+ private CompiledFilter(Set<String> filter) {
+ this.sourceSet = filter;
+ this.sourceSize = filter.size();
+ final Set<String> exact = new HashSet<>();
+ final List<String> prefixList = new ArrayList<>();
+ for (String key : filter) {
+ final String normalised = key.toLowerCase(Locale.ROOT);
+ if (normalised.endsWith("*")) {
+ prefixList.add(normalised.substring(0, normalised.length()
- 1));
+ } else {
+ exact.add(normalised);
+ }
+ }
+ this.exactKeys = exact;
+ this.prefixes = prefixList.toArray(new String[0]);
+ }
+
+ private boolean isFor(Set<String> filter) {
Review Comment:
```java
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
```java
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.
##########
core/src/test/java/org/apache/stormcrawler/util/MetadataTransferTest.java:
##########
@@ -152,4 +152,26 @@ void testFilterWithAsterisk() {
}
static class MyCustomTransferClass extends MetadataTransfer {}
+
+ @Test
+ void testWildcardPrefixIsCaseInsensitiveAndSelective() throws
MalformedURLException {
Review Comment:
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.
##########
core/src/main/java/org/apache/stormcrawler/util/MetadataTransfer.java:
##########
@@ -164,19 +168,86 @@ public Metadata filter(Metadata metadata) {
* with the prefix will be added.
*/
private Metadata filter(Metadata metadata, Set<String> filter) {
- Metadata filteredMetadata = new Metadata();
+ final CompiledFilter compiled = compile(filter);
+ final Map<String, String[]> source = metadata.asMap();
+ final Map<String, String[]> target = new HashMap<>();
+
+ // exact keys: direct lookups
+ for (String key : compiled.exactKeys) {
+ final String[] values = source.get(key);
+ if (values != null && values.length > 0) {
+ target.put(key, values);
+ }
+ }
- for (String key : filter) {
- if (key.endsWith("*")) {
- String prefix = key.substring(0, key.length() - 1);
- for (String k : metadata.keySet(prefix)) {
- metadata.copy(filteredMetadata, k);
+ // wildcards: a single pass over the metadata for all the prefixes,
+ // without allocating an intermediate key set per prefix
+ 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)) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
