[fix][broker] Execute per-topic entry filters with the same classloader by nicoloboschi · Pull Request #19364 · apache/pulsar

nicoloboschi · 2023-01-30T22:00:33Z

Fixes #19361

Motivation

Currently the entry filters per-topic/per-namespace are recreated from scratch (search in fs, unzip nar, classloader creation) every time a topic with entry filters enabled is created/updated.
This leads to multiple issues:

The topic creation path is affected by the time for looking up the file system and the unpacking (cpu intensive)
Each topic has his own NarClassLoader instance that has a relevant memory footprint impact

Modifications

Made the EntryFilterProvider a stateful class. In the constructor all the entry filter definitions and NarClassLoader are loaded, together with the broker entry filters
When the topic is created, using the same NarClassLoader a new EntryFilterWithClassLoader instance is created.

One different thing from before is that now the same entry filter class is loaded once. This means if a entry filter relies on static variables, those point to the same memory address for each EntryFilter invocation, even for different topics.
I think it's acceptable because:

This feature has never been released so it's not a real breaking change
The behaviour of EntryFilter is consistent with the broker ones. For the developer this means to not have to think to different contexts based on how the filter is deployed.

Other things in the pull request:

Moved the entryFilter update in AbstractTopic to the initialize method
Improved the entry filters override logic where, if the broker denied to override the broker entry filters but not filters were configured, then the topic's filter was used. This is wrong since the Pulsar admin doesn't expect any filter to be executed if allowOverrideEntryFilters is false
Added test about execution ordering of entry filters, if more than one. This is very important since it can cause performance regression based on the chain of entry filters configured.

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

lhotari

Good work @nicoloboschi . I have a comment about EntryFilterWithClassLoader.

lhotari · 2023-01-31T06:09:17Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterProvider.java

+        }
+    }
+
+    public List<EntryFilterWithClassLoader> loadEntryFiltersForPolicy(EntryFilters policy)


In the original implementation, EntryFilterWithClassLoader is unnecessarily used as a top level concept. I think that this is a bad approach. It's sufficient to use EntryFilter.

Please refactor the solution to hide EntryFilterWithClassLoader. It's possible that it could be removed completely.

Unfortunately we need to keep a reference to the NarClassLoader, this is a pattern we have in other plugins that can be loaded from a Nar file

lhotari · 2023-01-31T06:15:53Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/plugin/FilterEntryTest.java

+        EntryFilterWithClassLoader mockFilterReject = mock(EntryFilterWithClassLoader.class);
+        when(mockFilterReject.filterEntry(any(Entry.class), any(FilterContext.class))).thenReturn(
+                EntryFilter.FilterResult.REJECT);
+        EntryFilterWithClassLoader mockFilterAccept = mock(EntryFilterWithClassLoader.class);
+        when(mockFilterAccept.filterEntry(any(Entry.class), any(FilterContext.class))).thenReturn(
+                EntryFilter.FilterResult.ACCEPT);
+        if (overrideBrokerEntryFilters) {
+            setMockFilterToTopic((PersistentTopic) pulsar.getBrokerService()
+                    .getTopicReference(topic).get(), List.of(mockFilterReject, mockFilterAccept));
+        } else {
+            setMockFilterToTopic((PersistentTopic) pulsar.getBrokerService()
+                    .getTopicReference(topic).get(), List.of());
+            setMockBrokerFilter(List.of(mockFilterReject, mockFilterAccept));
+        }


I think that we need to get rid of this type of mocking. It's not that this PR has introduced this approach.

It would be useful it the EntryFilterProvider could be overridden in tests. It becomes easier if EntryFilterProvider is a simple interface and the existing EntryFilterProvider is renamed to EntryFilterProviderImpl or DefaultEntryFilterProvider.

Instead of injecting fields and doing other hacks, filters could simply be tested by passing an EntryFilterProvider in tests.

lhotari · 2023-01-31T06:18:11Z

Currently the entry filters per-topic/per-namespace are recreated from scratch (search in fs, unzip nar, classloader creation) every time a topic with entry filters enabled is created/updated.

One clarification to this: this applies to all entry filters in the master branch. The master branch has conflicting changes, but it will also load per-broker filters using the same logic as per-topic filters. The filters just won't get used at all unless allowOverrideEntryFilters=true.

The broker config level value gets set to the policies here:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java

Lines 385 to 386 in 785fb82

    
           topicPolicies.getEntryFilters().updateBrokerValue(new EntryFilters(String.join(",", 
        
                   config.getEntryFilterNames())));

Since #19361 applies to all entry filter usage, I'd suggest renaming the title of this PR.

lhotari · 2023-01-31T06:22:20Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/plugin/FilterEntryTest.java

+    private void setMockFilterToTopic(PersistentTopic topicRef, List<EntryFilterWithClassLoader> mockFilter) throws NoSuchFieldException, IllegalAccessException {
+        Field field = topicRef.getClass().getSuperclass().getDeclaredField("entryFilters");
+        field.setAccessible(true);
+        field.set(topicRef, mockFilter);
+    }
+
+    private void setMockBrokerFilter(List<EntryFilterWithClassLoader> mockFilter) throws NoSuchFieldException, IllegalAccessException {
+        Field field2 = pulsar.getBrokerService().getEntryFilterProvider()
+                .getClass().getDeclaredField("brokerEntryFilters");
+        field2.setAccessible(true);
+        field2.set(pulsar.getBrokerService().getEntryFilterProvider(), mockFilter);
+    }


I hope that there's a way to get rid of these as explained in my other comment.

eolivelli

LGTM

@hangc0276 please take a look

eolivelli · 2023-01-31T11:04:16Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterProvider.java

+        }
+    }
+
+    public List<EntryFilterWithClassLoader> loadEntryFiltersForPolicy(EntryFilters policy)


Unfortunately we need to keep a reference to the NarClassLoader, this is a pattern we have in other plugins that can be loaded from a Nar file

lhotari · 2023-01-31T16:21:59Z

Unfortunately we need to keep a reference to the NarClassLoader, this is a pattern we have in other plugins that can be loaded from a Nar file

@eolivelli That can be achieved without making EntryFilterWithClassLoader the top level type. The top level type should be simply EntryFilter. It's an implementation detail that doesn't need to be exposed if there happens to be such behavior that the NarClassloader gets closed when the entry filter gets closed.

nicoloboschi · 2023-01-31T21:23:03Z

@lhotari @eolivelli I've updated the pull:

moved the internal entriesFilter update on policies change callback
moved the top level class to EntryFilter
created a mock version of EntryFilterProvider. unfortunately the test class is not using ´TestPulsarService´ (yet [improve][test] Migrate tests to use PulsarTestContext #19376) so I had to inject it in the brokerService.

For the test part, I'm planning to improve the mocking part after #19376 gets merged, does it make sense to you @lhotari ?

lhotari · 2023-01-31T23:38:56Z

For the test part, I'm planning to improve the mocking part after #19376 gets merged, does it make sense to you @lhotari ?

@nicoloboschi sounds good!

lhotari

Good work @nicoloboschi

nicoloboschi · 2023-02-01T08:11:09Z

/pulsarbot rerun-failure-checks

eolivelli

Nice work

I left some additional comments

eolivelli · 2023-02-01T13:56:55Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

                                        });
-                            } catch (PulsarServerException e) {
-                                log.warn("Failed to create topic {}-{}", topic, e.getMessage());
+                            } catch (Throwable e) {


Catching Throwable is usually a code smell (and you should always deal with InterruptedException unfortunately.

Maybe we can move to RuntimeException here

eolivelli · 2023-02-01T13:57:39Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java


-        this.updateResourceGroupLimiter(Optional.of(data));
+        final Optional<Policies> optionalData = Optional.of(data);
+        this.updateResourceGroupLimiter(optionalData);


this change seems unrelated

eolivelli · 2023-02-01T13:58:35Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterProvider.java

-                builder.put(filterName, filter);
-            }
-            log.info("Successfully loaded entry filter for name `{}`", filterName);
+            final EntryFilter filterWithClassLoader = load(metaData);


nit: rename to "entryFilter"

eolivelli · 2023-02-01T14:00:15Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterProvider.java

+    }
+
+    private static String classLoaderKey(Path archivePath) {
+        return archivePath.toFile().getAbsolutePath();


is archivePath.toFile().getAbsolutePath() potentially performing some IO operation ? (metadata access?)

probably archivePath.toString() is enough in this context

eolivelli · 2023-02-01T14:01:01Z

...broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterWithClassLoader.java


 @Slf4j
+@ToString
 public class EntryFilterWithClassLoader implements EntryFilter {


do we still need this class ?

yes, it's the implementation of EntryFilter

eolivelli · 2023-02-01T14:02:45Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterProvider.java

+    @VisibleForTesting
+    protected Map<String, EntryFilterMetaData> definitions;
+    @VisibleForTesting
+    protected Map<String, NarClassLoader> cachedClassLoaders;


is it required that we refer to NarClassLoader here ? can it be simply a "ClassLoader" ?

no because we need to get the ServiceDefinition file from the classloader, so a NarClassLoader is required

eolivelli · 2023-02-01T14:03:59Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/testcontext/MockEntryFilterProvider.java

+            meta.setDefinition(def);
+            meta.setArchivePath(Path.of(name));
+            definitions.put(name, meta);
+            final NarClassLoader ncl = mock(NarClassLoader.class);


if cachedClassLoaders is not required to old " NarClassLoader" entries that maybe this code can be simplified a lot.
and you can create a new URLClassloader with Thread.currentThread().getContextClassLoader() as parent
and so we continue to remove mocks

see my previous comment

eolivelli

Lgtm

### Modifications - Upgrade the Pulsar dependency to 3.0.0.1-SNAPSHOT - Use `List<EntryFilter>` from apache/pulsar#19364 as the filters - Remove the removed APIs in `AuthorizationService` - Use `TopicType` instead of String.

### Modifications - Upgrade the Pulsar dependency to 3.0.0.1-SNAPSHOT - Use `List<EntryFilter>` from apache/pulsar#19364 as the filters - Remove the removed APIs in `AuthorizationService` - Use `TopicType` instead of String. - Fix authenticator failure caused by apache/pulsar#19295 Co-authored-by: Demogorgon314 <kwang@streamnative.io>

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 30, 2023

lhotari requested changes Jan 31, 2023

View reviewed changes

lhotari reviewed Jan 31, 2023

View reviewed changes

eolivelli approved these changes Jan 31, 2023

View reviewed changes

nicoloboschi force-pushed the fix-entry-filters branch from 632b474 to b1ed5f8 Compare January 31, 2023 21:36

nicoloboschi mentioned this pull request Jan 31, 2023

[run-tests] [fix][broker] Execute per-topic entry filters with the same classloader nicoloboschi/pulsar#51

Closed

lhotari approved these changes Jan 31, 2023

View reviewed changes

nicoloboschi added the ready-to-test label Feb 1, 2023

eolivelli requested changes Feb 1, 2023

View reviewed changes

eolivelli approved these changes Feb 1, 2023

View reviewed changes

[fix][broker] Execute per-topic entry filter with the same classloader

3b6153c

nicoloboschi force-pushed the fix-entry-filters branch from 600b4a8 to 3b6153c Compare February 1, 2023 16:14

nicoloboschi added 2 commits February 1, 2023 18:26

style

bfe104d

fix test

2a46e3f

nicoloboschi merged commit a4c3034 into apache:master Feb 1, 2023

nicoloboschi added this to the 3.0.0 milestone Feb 1, 2023

nicoloboschi self-assigned this Feb 1, 2023

BewareMyPower mentioned this pull request Mar 6, 2023

Fix the incompatibility with Pulsar 3.0.0.1-SNAPSHOT streamnative/kop#1759

Merged

3 tasks

eolivelli mentioned this pull request May 23, 2024

[fix][broker] EntryFilters fix NoClassDefFoundError due to closed classloader #22767

Merged

14 tasks

Conversation

nicoloboschi commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

lhotari 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!

lhotari commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhotari commented Jan 31, 2023

Uh oh!

nicoloboschi commented Jan 31, 2023

Uh oh!

lhotari commented Jan 31, 2023

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

nicoloboschi commented Feb 1, 2023

Uh oh!

eolivelli 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!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nicoloboschi commented Jan 30, 2023 •

edited

Loading

lhotari commented Jan 31, 2023 •

edited

Loading