Event type IDs + event metadata incl. OTel context #2998
Conversation
adnanhemani
left a comment
adnanhemani
left a comment
There was a problem hiding this comment.
Overall, looks great. Thank you for this, @adutra! One question that I think might help make this a tighter change, but overall no concerns!
| polarisEventListener.onBeforeCreateCatalog( | ||
| new CatalogsServiceEvents.BeforeCreateCatalogEvent(request.getCatalog().getName())); | ||
| new CatalogsServiceEvents.BeforeCreateCatalogEvent( | ||
| PolarisEvent.createEventId(), |
There was a problem hiding this comment.
Wanted to see your thoughts on this: can we collapse Event ID into the EventMetadata as well? I think it makes sense as part of the "metadata" of the event and may help us save many lines of code uniformly across all events.
There was a problem hiding this comment.
I thought about that too, but after some hesitation, I think an ID is a bit more than "metadata". The fact that this specific field must be unique makes it imho a bit special. Wdyt?
That said I don't have strong opinions on this, and I'd be OK to move it to metadata if there is a consensus to do so.
There was a problem hiding this comment.
I understand your point, it is definitely a bit of a "grey area." I think the amount of cleanliness that we gain from moving it into EventMetadata outweighs the ambiguities here, though. I'm also willing to sway my opinion with the will of the community, but I would vote for merging it into EventMetadata if others feel similarly :)
There was a problem hiding this comment.
The more I think about this the more I think we shouldn't move the id to the metadata. Here is what the PolarisEventMetadata would look like:
@PolarisImmutable
public interface PolarisEventMetadata {
UUID id();
// other methods omitted for brevity
}But that creates an ambiguity: is id() the identifier of the metadata object, or of the event object?
I could rename it to eventId() of course, but it still doesn't feel right to me. An identifier should be attached to the object it identifies.
This PR implements the action items from the following discussion threads: - https://lists.apache.org/thread/yx7pkgczl6k7bt4k4yzqrrq9gn7gqk2p - https://lists.apache.org/thread/rl5cpcft16sn5n00mfkmx9ldn3gsqtfy - https://lists.apache.org/thread/5dpyo0nn2jbnjtkgv0rm1dz8mpt132j9 Summary of changes: - Introduced a `PolarisEventType` enum holding the 150+ event types. - Introduced a `PolarisEventMetadata` interface as suggested by @adnanhemani, exposing: timestamp, realm ID, principal, request ID, and OTel context. - Introduced a `PolarisEventMetadataFactory` to centralize the logic for gathering the various elements of an event metadata. - Modified `PolarisEvent` to expose 3 new methods: - `UUID id()` - `PolarisEventType type()` - `PolarisEventMetadata metadata()` - Persistence of OTel context is done in `additional_properties` as suggested by @flyrain. - Added `InMemoryBufferEventListenerIntegrationTest` to verify that all contextual data is properly persisted.
adutra
force-pushed
the
events-api-refactoring
branch
from
1fd437e to
c84c18a
Compare
|
@adutra - I see you force-pushed the last commit so it's rewritten the commit history. I'm assuming it was for rebasing this PR onto main? Was there any other major change? Since it's a larger change, I fear I may have missed something. |
I rebased because of conflicts - no functional change. |
2 participants
This PR implements the action items from the following discussion threads:
Summary of changes:
PolarisEventTypeenum holding the 150+ event types.PolarisEventMetadatainterface as suggested by @adnanhemani, exposing: timestamp, realm ID, principal, request ID, and OTel context.PolarisEventMetadataFactoryto centralize the logic for gathering the various elements of an event metadata.PolarisEventto expose 3 new methods:UUID id()PolarisEventType type()PolarisEventMetadata metadata()additional_propertiesas suggested by @flyrain.InMemoryBufferEventListenerIntegrationTestto verify that all contextual data is properly persisted.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)