Skip to content

Conversation

@sacsar
Copy link
Contributor

@sacsar sacsar commented Dec 17, 2021

We add an emitGeneralAuditEvents field to BaseLocalDAO which enables
the sending of the general all-aspect audit events to be disabled,
assuming that aspect-specific events are enabled.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

We add an `emitGeneralAuditEvents` field to BaseLocalDAO which enables
the sending of the general all-aspect audit events to be disabled,
assuming that aspect-specific events are enabled.
/**
* Enables or disables the emission of general audit events.
*/
public void enableGeneralAuditEvent(boolean generalAuditEventEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this method public? If it's public then what is the difference between this method and setEmitGeneralAuditEvent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No difference :) I was trying to be consistent in the naming and it turned out it wasn't possible--there's enableModelValidationOnWrite, but setEmitAspectSpecificAuditEvent. I thought enableGeneralAuditEvent was the "better" name, but added the other for symmetry with setEmitAspectSpecificAuditEvent. I can delete one or the other or, if you think enableFoo is the way forward, mark the setFoo one as deprecated and note that it just routes to enableFoo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just stick with one method, enableGeneralAuditEvent sounds good to me.

@sacsar sacsar requested a review from jywadhwani February 2, 2022 15:15
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.

3 participants