Skip to content

Conversation

adnanhemani
Copy link
Contributor

This PR adds the Events instrumentation for the Policy Service APIs, surrounding the default delegated call to the business logic APIs.

@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Aug 31, 2025
@adnanhemani adnanhemani marked this pull request as ready for review August 31, 2025 06:26
@HonahX HonahX assigned HonahX and unassigned HonahX Sep 2, 2025
@HonahX HonahX self-requested a review September 2, 2025 16:48
return resp;
}

private String getCatalogFromPrefix(String prefix, RealmContext realmContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: inline the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

public class CatalogPolicyServiceEvents {

// Policy CRUD Events
public record BeforeCreatePolicyEvent(
Copy link
Contributor

@adutra adutra Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to insist on naming, but it's still inconsistent.

In #2482 we have (Before|After)(Object)(Verb)Event. E.g. BeforeCatalogListEvent.

Here we have (Before|After)(Verb)(Object)Event. E.g. BeforeCreatePolicyEvent.

(I find verb-object more natural than object-verb btw, but I'll defer the decision to native English speakers.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it again, I think verb-object is better too. Let me keep these events as-is in that case and change the rest of the PRs.

@adnanhemani adnanhemani force-pushed the ahemani/event_instrumentation_catalog_policy_service branch from 16960b3 to b4777fd Compare September 3, 2025 21:08
HonahX
HonahX previously approved these changes Sep 4, 2025
Copy link
Contributor

@HonahX HonahX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Do we have plan to add tests that verify all the before and after are correctly called during each request?

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 4, 2025
adutra
adutra previously approved these changes Sep 4, 2025
@adutra
Copy link
Contributor

adutra commented Sep 4, 2025

Do we have plan to add tests that verify all the before and after are correctly called during each request?

+1 to the idea.

@adnanhemani adnanhemani dismissed stale reviews from adutra and HonahX via b012081 September 5, 2025 21:00
@singhpk234 singhpk234 merged commit b8d210a into apache:main Sep 5, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants