adutra commented on code in PR #4064:
URL: https://github.com/apache/polaris/pull/4064#discussion_r3045303245
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisCatalogsEventServiceDelegator.java:
##########
@@ -60,52 +60,64 @@ public class PolarisCatalogsEventServiceDelegator
implements PolarisCatalogsApiS
@Override
public Response createCatalog(
CreateCatalogRequest request, RealmContext realmContext, SecurityContext
securityContext) {
- polarisEventDispatcher.dispatch(
- new PolarisEvent(
- PolarisEventType.BEFORE_CREATE_CATALOG,
- eventMetadataFactory.create(),
- new EventAttributeMap()
- .put(EventAttributes.CATALOG_NAME,
request.getCatalog().getName())));
+ if
(polarisEventDispatcher.hasListeners(PolarisEventType.BEFORE_CREATE_CATALOG)) {
Review Comment:
Some more thoughts on this:
The if-block approach was designed initially with serialization in mind.
IOW, the event was created with lots of serialized attributes (either JSON or
`toString()` calls - I don't recall exactly). So, creating an event could be
expensive.
But now that we have the events attribute map, creating a `PolarisEvent` has
become much cheaper — it's just a wrapper around a thin map of references. The
if block is now avoiding roughly:
- One `PolarisEvent` object (~32 bytes)
- One `EventAttributeMap` object + its internal map (~200 bytes)
- A handful of `Map.Entry` objects for the `.put()` calls
All of these are short-lived. It's not nothing, but it's not the end of the
world either.
One edge case where the guard could matter is if an attribute value is
computed lazily solely for the event, e.g., a `.toString()` that creates a new
object. Looking at the call sites, I don't think it's happening today (**but it
could, in the future**).
The `Supplier<PolarisEvent>` approach imho adds more overhead in the happy
path (when there are listeners) to save a small memory overhead in the
no-listeners path. All in all, it's not a perfect solution either.
So just to summarize, I'd still stick with the if block, mostly because of
the risk of having some expensive-to-compute attribute added to the event.
--
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]
