Hi all, (Sorry for the cross-posting)
I've just opened a PR [1] that implements contextual information for events, as discussed here, and also event type IDs, as discussed in another discussion thread [2]. The reason why I openen one PR for both is that both enhancements affect exactly the same classes and 2 PRs would conflict with each other and force reviewers to review the same places twice. Thanks, Alex [1]: https://github.com/apache/polaris/pull/2998 [2]: https://lists.apache.org/thread/yx7pkgczl6k7bt4k4yzqrrq9gn7gqk2p On Tue, Oct 28, 2025 at 5:49 PM Alexandre Dutra <[email protected]> wrote: > > Hi all, > > I am going to assume that we have reached a consensus on this topic > and I will start working on it. > > If someone disagrees, please chime in. > > Thanks, > Alex > > Thanks, > Alex > > On Sun, Oct 26, 2025 at 12:53 PM Alexandre Dutra <[email protected]> wrote: > > > > Hi Adnan, > > > > Sure, EventMetadata works for me. > > > > I think we are moving into implementation details a bit prematurely > > though. We don't need the exact final design right now, as long as we > > can demonstrate – as we did – that what's proposed can be easily > > achieved. > > > > Thanks, > > Alex > > > > On Sat, Oct 25, 2025 at 11:44 AM Adnan Hemani > > <[email protected]> wrote: > > > > > > Hi Alex, > > > > > > Got it - with this suggestion, we will have to add this information > > > manually into the Delegators (which are where we are emitting the > > > events). Personally, this felt like a lot of repeated code for me that > > > would be mixed in with the important parts of crafting the event and I > > > was hoping not to crowd that logic - which is why I had pushed it out to > > > the Event Listeners to take care of this there. This way, we were keeping > > > the Java records crisp and not having to add the copy-paste logic into > > > the event during event emission time. > > > > > > I fully agree with the philosophy that id, timestamp, and user > > > information should be part of a PolarisEvent. But I still don’t think > > > this suggested change is a great way to go, even though it may be the > > > best option we have at the moment - so I’m torn. > > > > > > One thought to try to make the repeated code concise is to use an > > > “EventMetadata” record that would become the parent of the id, timestamp, > > > and user fields - and have the “EventMetadata” record be part of the > > > PolarisEvent interface. We can then add a quick helper method wherever > > > required to generate a new “EventMetadata” record - and we can limit the > > > bloating of the Event record variables. Any further PolarisEvent-wide > > > variables should be then added to the “EventMetadata” record as well. I > > > know this option is not ideal either - but presents the functionality at > > > the least impact to the code that I can see. WDYT? (Sample code in > > > Appendix, if it’s helpful :) > > > > > > Best, > > > Adnan Hemani > > > > > > —— > > > Appendix: > > > > > > public record EventMetadata(UUID uuid, Instant timestamp, > > > PolarisPrincipal user); > > > > > > public interface PolarisEvent { > > > EventMetadata eventMetadata(); > > > default static UUID createEventId() { return UUID.randomUUID(); } > > > } > > > > > > public record BeforeSomethingEvent(String someParam, EventMetadata em) > > > implements PolarisEvent {} > > > > > > Instantiation: > > > > > > @Inject SecurityContext securityContext; > > > @Inject Clock clock; > > > > > > private EventMetadata createEventMetadata() { > > > return new EventMetadata(PolarisEvent.createEventId(), clock.instant(), > > > (PolarisPrincipal) securityContext.getUserPrincipal()); > > > } > > > > > > new BeforeSomethingEvent(“abcd”, createEventMetadata()); > > > > > > > On Oct 24, 2025, at 2:40 AM, Alexandre Dutra <[email protected]> wrote: > > > > > > > > Hi Adnan, > > > > > > > > I agree that Java records lack some flexibility but I think we can > > > > achieve a nice layout. Here is a tentative design: > > > > > > > > public interface PolarisEvent { > > > > UUID id(); > > > > Instant timestamp(); > > > > PolarisPrincipal user(); > > > > static UUID createEventId() { return UUID.randomUUID(); } > > > > } > > > > > > > > Here is a typical event record: > > > > > > > > public record BeforeSomethingEvent( > > > > UUID id, > > > > Instant timestamp, > > > > PolarisPrincipal user, > > > > String someOtherParameter) > > > > implements PolarisEvent {} > > > > > > > > And here is a typical event instantiation idiom: > > > > > > > > @Inject SecurityContext securityContext; > > > > @Inject Clock clock; > > > > > > > > new BeforeSomethingEvent( > > > > PolarisEvent.createEventId(), > > > > clock.instant(), > > > > (PolarisPrincipal) securityContext.getUserPrincipal(), > > > > "someParameter"); > > > > > > > > What do you think of the above? > > > > > > > > Thanks, > > > > Alex > > > > > > > > On Fri, Oct 24, 2025 at 10:15 AM Adnan Hemani > > > > <[email protected]> wrote: > > > >> > > > >> Hi Alex, > > > >> > > > >> I had thought about this earlier and abandoned the idea due to > > > >> implementation concerns. So, my question is how exactly will these > > > >> methods be implemented and used? > > > >> > > > >> Let’s take id() as the talking point, but similar points can be > > > >> applied to timestamp(). PolarisEvent is an interface and all > > > >> implementations of this interface are Java records - as a result, > > > >> neither PolarisEvent nor its implementations can contain non-static > > > >> instance fields. So if a PolarisEvent cannot save its ID through a > > > >> non-static instance field, how do we guarantee idempotency if id() is > > > >> called twice on an instance of PolarisEvent? > > > >> > > > >> The other way I see is we instead include id, timestamp, and user as > > > >> named arguments of every record signature along with a custom > > > >> constructor which will generate the new ID and timestamp - which makes > > > >> the code extremely messy IMO (and was one of the original reasons why > > > >> we stuck to using an Interface for PolarisEvent rather than switching > > > >> over to an ABC, which I had proposed earlier). Or we go towards using > > > >> an intermediate ABC with PolarisEvent re-written as a Sealed > > > >> Interface, with each record extending the intermediate ABC. I’m not > > > >> sure I understand the ramifications of doing this yet, so cannot > > > >> endorse this approach either. > > > >> > > > >> Did I miss some other way to make this work? Or were you thinking of > > > >> something completely different? > > > >> > > > >> Best, > > > >> Adnan Hemani > > > >> > > > >>> On Oct 23, 2025, at 10:18 AM, Alexandre Dutra <[email protected]> > > > >>> wrote: > > > >>> > > > >>> Hi all, > > > >>> > > > >>> Just to be clear, and sorry for the confusion: the idea was to add > > > >>> those methods to the PolarisEvent interface, not PolarisEventListener. > > > >>> > > > >>> I agree with Mike that PolrisEventListener shouldn't deal with context > > > >>> data. In my mind, each PolarisEvent instance would carry its own ID, > > > >>> timestamp (at creation time) and user (the principal that triggered > > > >>> the event). > > > >>> > > > >>> Thanks, > > > >>> Alex > > > >>> > > > >>> On Thu, Oct 23, 2025 at 7:08 PM Michael Collado > > > >>> <[email protected]> wrote: > > > >>>> > > > >>>> +1 on adding the described fields to the PolarisEvent. > > > >>>> > > > >>>> I don't think the PolarisEventListener is really the right place to > > > >>>> add any > > > >>>> default methods about context, though. I'm not sure if you meant > > > >>>> adding > > > >>>> those to the PolarisEvent interface so that existing records have a > > > >>>> default > > > >>>> value? > > > >>>> > > > >>>> Mike > > > >>>> > > > >>>> On Wed, Oct 22, 2025 at 10:26 PM Jean-Baptiste Onofré > > > >>>> <[email protected]> > > > >>>> wrote: > > > >>>> > > > >>>>> Hi Alex > > > >>>>> > > > >>>>> It makes sense to me. > > > >>>>> > > > >>>>> Are you also proposing to have default methods in the interface ? > > > >>>>> > > > >>>>> interface PolarisEventListener { > > > >>>>> > > > >>>>> default UUID id() { ... } > > > >>>>> > > > >>>>> default Instant timestamp() { ... } > > > >>>>> > > > >>>>> default PolarisPrincipal user() { ... } > > > >>>>> > > > >>>>> } > > > >>>>> > > > >>>>> Maybe we can have a default behavior that implementations can > > > >>>>> "override" if needed. > > > >>>>> > > > >>>>> Regards > > > >>>>> JB > > > >>>>> > > > >>>>> On Wed, Oct 22, 2025 at 5:08 PM Alexandre Dutra <[email protected]> > > > >>>>> wrote: > > > >>>>>> > > > >>>>>> Hi all, > > > >>>>>> > > > >>>>>> We have two (albeit incomplete) PolarisEventListener > > > >>>>>> implementations > > > >>>>>> today: one for JDBC and one for AWS CloudWatch. > > > >>>>>> > > > >>>>>> Both enhance event payloads internally by adding a timestamp and > > > >>>>>> user > > > >>>>>> data. These two attributes are indeed very common in event-oriented > > > >>>>>> systems. > > > >>>>>> > > > >>>>>> The JDBC implementation also generates unique IDs internally. > > > >>>>>> That's > > > >>>>>> because the PolarisEvent interface lacks an id() method, despite > > > >>>>>> the > > > >>>>>> common need for event disambiguation. > > > >>>>>> > > > >>>>>> Therefore, I propose promoting these three attributes (ID, > > > >>>>>> timestamp, > > > >>>>>> user) to the PolarisEvent interface, by adding the following > > > >>>>>> methods: > > > >>>>>> > > > >>>>>> UUID id(); > > > >>>>>> Instant timestamp(); > > > >>>>>> PolarisPrincipal user(); > > > >>>>>> > > > >>>>>> This information is readily available at event instantiation sites, > > > >>>>>> and can be provided via CDI injection without hassle. > > > >>>>>> > > > >>>>>> Let me know your thoughts. > > > >>>>>> > > > >>>>>> Thanks, > > > >>>>>> Alex > > > >>>>> > > > >> > > >
