-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(): Adding supports for data incidents for datasets, dashboards, charts, data jobs, data flows #9710
feat(): Adding supports for data incidents for datasets, dashboards, charts, data jobs, data flows #9710
Conversation
edb7de0
to
2cb5b64
Compare
a3ccceb
to
6c4d311
Compare
4f9992c
to
a4ad261
Compare
…, and incidents fields on entities
… required inside incidents tab
3861541
to
7699caa
Compare
_entityClient = entityClient; | ||
_graphClient = graphClient; | ||
_timeseriesAspectService = timeseriesAspectService; | ||
_statusCache = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense. Why include an empty cache here?
try { | ||
final Filter filter = buildIncidentsEntityFilter(entityUrn, IncidentState.ACTIVE.toString()); | ||
final SearchResult searchResult = | ||
_entityClient.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If caching is needed, the systemEntityClient and a slight variation of the call should provide that cache without having to maintain a separate cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, the cache is on the higher level object including data from those other services. Disregard the comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im removing the cache. it's only really required in OSS because it's expensive to compute assertion health. but its not being used anyways so im removing as you've mentioned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, just the one comment about the odd empty cache.
Summary
In this PR, we are adding support for Data Incidents on Datasets, Dashboards, Charts, Data Jobs, Data Flows.
Capabilities:
Also included the docs updates for docs website, including new incidents feature guide doc, and updates to policies guides.
Screenshots
Checklist
dded/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.