Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: fix top queries table row height #565

Merged
merged 6 commits into from
Oct 24, 2023
Merged

fix: fix top queries table row height #565

merged 6 commits into from
Oct 24, 2023

Conversation

krosy1337
Copy link
Contributor

No description provided.

@krosy1337 krosy1337 force-pushed the fix-top-queries-in-cpu branch 2 times, most recently from 152aad3 to 8aba9b7 Compare October 19, 2023 15:19
@krosy1337 krosy1337 marked this pull request as ready for review October 19, 2023 15:20
@krosy1337 krosy1337 force-pushed the fix-top-queries-in-cpu branch 2 times, most recently from a7d92a6 to 9b6beb7 Compare October 23, 2023 10:20
@artemmufazalov
Copy link
Member

  1. I think QueryHash will be a better naming. QueryId will intersects with queries in other our services, it supposes we have some queries ids stored somewhere, so there should be a functionality to track progress of long queries and retrieve results. QueryId isn't supposed to be calculated on a go and in UI

  2. Negative ids looks weird for me

Screen Shot 2023-10-23 at 13 48 14

@krosy1337 krosy1337 force-pushed the fix-top-queries-in-cpu branch 2 times, most recently from 60b7b79 to 2d2599c Compare October 23, 2023 13:40
artemmufazalov
artemmufazalov requested changes Oct 23, 2023
View reviewed changes
src/containers/Tenant/Diagnostics/TopQueries/getTopQueriesColumns.tsx Outdated
render: ({row}) =>
// We use unsigned right shift operator (>>>) to avoid negative values
// eslint-disable-next-line no-bitwise
(crc32.str(String(row.QueryText)) >>> 0).toString(16).toUpperCase().padStart(8, '0'),
Copy link
Member

Choose a reason for hiding this comment

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

Can you please make it a separate function in utils? And could you please split it to steps and explain every step in comments

  1. crc32
  2. >>>>
  3. toString(16)
  4. toUpperCase()
  5. padStart(8, '0')

@krosy1337 krosy1337 force-pushed the fix-top-queries-in-cpu branch from 4769cdb to 699230d Compare October 24, 2023 13:04
@krosy1337 krosy1337 merged commit b12dceb into main Oct 24, 2023
4 checks passed
@krosy1337 krosy1337 deleted the fix-top-queries-in-cpu branch October 24, 2023 13:17
@ydb-platform-bot ydb-platform-bot mentioned this pull request Oct 24, 2023
Merged
This was referenced Jan 31, 2024
Closed
Closed
Closed
Merged
@ydb-platform-bot ydb-platform-bot mentioned this pull request Jun 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artemmufazalov artemmufazalov artemmufazalov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@krosy1337 @artemmufazalov