[Feature] TopNQueries - WLM Intergation #432
Conversation
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
KishoreKicha14
requested review from
ansjcy,
deshsidd,
dzane17 and
jainankitk
as code owners
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
KishoreKicha14
changed the title
ansjcy
left a comment
ansjcy
left a comment
There was a problem hiding this comment.
Overall looks good, I found some small bugs and also have concerns on backward compatibilties.
After resolving the comments, could you also make sure validate and attach screenshots on MDS support? Especially using this dashboards in 3.4 and connect to 2.19, 3.1, 3.3 data sources.
types/types.ts
Outdated
| task_resource_usages: Task[]; | ||
| id: string; | ||
| group_by: string; | ||
| wlm_group_id: string; |
There was a problem hiding this comment.
could this be null or undefined? if WLM is disabled, or especially when we query on old top queries indices that do not have this field defined yet
There was a problem hiding this comment.
Update code to handle null or undefined
| useEffect(() => { | ||
| const checkWlmSupport = async () => { | ||
| try { | ||
| const { getVersionOnce, isVersion33OrHigher } = await import('../../utils/version-utils'); |
There was a problem hiding this comment.
why do we need to do dynamic import here?
There was a problem hiding this comment.
now we have static import
| const res = await core.http.get(API_ENDPOINTS.WLM_WORKLOAD_GROUP, { query: httpQuery }); | ||
| return res && typeof res === 'object' && Array.isArray(res.workload_groups); | ||
| } catch (e) { | ||
| return false; |
There was a problem hiding this comment.
are there any specific errors we care about? Currently we silently swallows all errors without logging
There was a problem hiding this comment.
updated to log the errors
| name: WLM_GROUP, | ||
| render: (wlmGroupId: string, q: SearchQueryRecord) => { | ||
| if (q.group_by === 'SIMILARITY') return '-'; | ||
| const groupId = wlmGroupId || 'DEFAULT_WORKLOAD_GROUP'; |
There was a problem hiding this comment.
Consider using a constant for DEFAULT_WORKLOAD_GROUP since you are using it multiple times in this PR
There was a problem hiding this comment.
add constant to constant.ts
| field: WLM_GROUP_FIELD as keyof SearchQueryRecord, | ||
| name: WLM_GROUP, | ||
| render: (wlmGroupId: string, q: SearchQueryRecord) => { | ||
| if (q.group_by === 'SIMILARITY') return '-'; |
There was a problem hiding this comment.
do we want to also wrap this in <span></span> instead of raw string?
|
|
||
| // Get wlmGroupId from URL parameters | ||
| const urlParams = new URLSearchParams(location.search); | ||
| const wlmGroupIdFromUrl = urlParams.get('wlmGroupId'); |
There was a problem hiding this comment.
Let's add some unit tests for this logic too
There was a problem hiding this comment.
updated unit test
| return Array.from(set).map((v) => ({ value: v, name: v, view: v.replaceAll('_', ' ') })); | ||
| }, [queries]); | ||
|
|
||
| const wlmGroupOptions = useMemo(() => { |
There was a problem hiding this comment.
Please add some comments on what this helper function do
There was a problem hiding this comment.
added comment
|
|
||
| // Set initial WLM group filter from URL | ||
| useEffect(() => { | ||
| if (wlmGroupIdFromUrl && !selectedWlmGroups.includes(wlmGroupIdFromUrl)) { |
There was a problem hiding this comment.
When navigating from WLM to Top queries page but will this also update the table filter UI? I see on your screenshot it is not updated.
| field: WLM_GROUP_FIELD as keyof SearchQueryRecord, | ||
| name: WLM_GROUP, | ||
| render: (wlmGroupId: string, q: SearchQueryRecord) => { | ||
| if (q.group_by === 'SIMILARITY') return '-'; |
There was a problem hiding this comment.
if group by is enabled, should we dynamically hide this column?
There was a problem hiding this comment.
but for the mixed type when both are selected or none selected the query list has both the types then we will see this column for group
| render: (wlmGroupId: string, q: SearchQueryRecord) => { | ||
| if (q.group_by === 'SIMILARITY') return '-'; | ||
| const groupId = wlmGroupId || 'DEFAULT_WORKLOAD_GROUP'; | ||
| const displayName = |
There was a problem hiding this comment.
There's a bug on this logic. If connected to data sources < 3.3, in which backend records does not have this wlmgroupid defined, we are still showing DEFAULT_WORKLOAD_GROUP for these records. Ideally they should be -.
There was a problem hiding this comment.
updated the logic to handle the below 3.3
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
2 participants
Description
This PR introduces WLM Group visibility in the Top Queries UI. A new “WLM Group” column is added to the table, along with full support for filtering and sorting. The implementation is backwards-compatible with existing Top Queries records that do not contain a WLM group field.
CASE 1: when WLM is available:
Navigation from wlm group to TopNQueries
After Movie workload group is deleted it show "-" and as a text
CASE 2: when WLM is not available:
It will not show link to navigate to WLM detail page
Issues Resolved
Closes [#153]
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.