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.

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: use VirtualTable for nodes #578

Merged
merged 3 commits into from
Oct 27, 2023
Merged

feat: use VirtualTable for nodes #578

merged 3 commits into from
Oct 27, 2023

Conversation

artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Oct 26, 2023

Currently VirtualTable is applied only to cluster Nodes. The reason - in Diagnostics we use compute that works badly, so we should replace it with nodes and only after VirtualTable could be applied there.

VirtualTable could be turned on with settings in experiments section "Use virtual table for cluster Nodes tab"

@@ -18,7 +18,6 @@

&_size_m {
position: relative;
top: 20%;
Copy link
Member Author

Choose a reason for hiding this comment

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

Because of this value content sometimes didn't fit in wrapper box and was partly hidden

Copy link
Member Author

Choose a reason for hiding this comment

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

Styles was partly taken from DataTable with plenty overwrites we made in the project

Copy link
Member Author

Choose a reason for hiding this comment

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

I made separate VirtualNodes component not to create a mess in original Nodes

@artemmufazalov artemmufazalov marked this pull request as ready for review October 26, 2023 15:06
@sareyu
Copy link
Collaborator

sareyu commented Oct 27, 2023

проверял на инстансе мониторинга. стало радикально лучше, круто

  • данные для фрейма загружаются даже в том случае, если они уже были загружены (если поскроллить вниз, потом вверх)
  • данные для фрейма грузятся сразу в момент попадания во вьюпорт (или дебаунс маленький) - в принципе не проблема, но можно попробовать потюнить
  • проценты залезают на шапку таблицы при сколлле
image
  • параметры сортировки и фильтров не прорастают в урл
  • ручки с сортировкой отрабатывают ощутимо медленно (например, по CPU)
  • при начальном заходе на страницу выполняются 3 одинаковых запроса
  • видимая задержка перед появлением скелетонов, если отмотать таблицу до низа и начать скроллить вверх
  • ненужный горизонтальный скролл на странице (последний столбец с прочерком очень широкий. зачем там расчет ширины столбцов вообще понадобился?)

sareyu
sareyu previously approved these changes Oct 27, 2023
}

// Display skeletons in case of error
if (chunkData.loading || chunkData.error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no, you should show error message while error

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an error in the next chunks, not in the first. So it's the case, when we have already loaded some data, and n chunk could not be loaded due to some reason. We didn't figured out how to show error for particular chunk, so there will be loading rows for a while

</svg>
);

function getSortIcon(order?: SortOrderType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://css-tricks.com/snippets/css/css-triangle/ :)
and maybe css:rotate instead of separate icon

return (
<colgroup>
{columns.map((column) => {
return <col key={column.name} style={{width: `${column.width}px`}} />;
Copy link
Collaborator

Choose a reason for hiding this comment

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

with width on <col /> element you don't need set size on each td below

dispatch(setChunkError(id, err as IResponseError));
setError(err as IResponseError);
}
}, DEFAULT_REQUEST_TIMEOUT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this timeout should be in debounced function, which needs to be reset on leave

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you please clarify, what you mean?

A make functions control manual, so we could always have one active load per chunk. And at the same time functions from different chunks should be debounced separately. So actually it won't be a single debounce function, but a separate function for every chunk. And for that case my setup looks easier than setup with lodash debounce

// Load chunks if they become active
// This mecanism helps to set chunk active state from different sources, but load data only once
// Only currently active chunks should be in state so iteration by the whole state shouldn't be a problem
useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

проверка на wasLoaded сейчас не работает как ожидается.
вообще не уверен, что обходить весь state каждый раз нужно - почему не сделать загрузку данных в onEntry, и там же проверять их наличие. на какие еще сценарии маркировки чанка активным ты закладываешься?
может разделить таки initial загрузку данных и refetch? не знаю, большая ли проблема, что проверка данных на загруженность сейчас не работает..

Copy link
Member Author

Choose a reason for hiding this comment

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

Проверка на загруженность сейчас нужна только для того, чтобы не запрашивать данные повторно для чанков, которые на экране, но запрос для которых уже отработал. То есть, перезапрос при скролле все равно есть и это намеренно, на данный момент мы решили не хранить данные, кроме тех, что есть на экране (+-50%)

@@ -0,0 +1,8 @@
export const getArray = (arrayLength: number) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[...Array(n).keys()]

@artemmufazalov
Copy link
Member Author

artemmufazalov commented Oct 27, 2023

  • данные для фрейма загружаются даже в том случае, если они уже были загружены (если поскроллить вниз, потом вверх)

На данный момент решили не хранить данные кроме тех, что уже на экране (+-50%)

  • параметры сортировки и фильтров не прорастают в урл

Такая проблема была, под это есть отдельный тикет

  • ручки с сортировкой отрабатывают ощутимо медленно (например, по CPU)

Дело в том, что для некоторых запросов на бэке все равно приходится обсчитывать все ноды кластера, или запрашивать данные из других сущностей. Поэтому что-то работает быстро - в основном сортировка по ID или текстовым полям, рассчитываемые поля работают медленно - проблема 17к нод никуда не пропадает, хотя теперь она уже на бэке

  • при начальном заходе на страницу выполняются 3 одинаковых запроса
  • видимая задержка перед появлением скелетонов, если отмотать таблицу до низа и начать скроллить вверх

Знаю. Пока не нашел как починить, решил сделать первую версию, а потом оптимизировать оставшееся.

  • ненужный горизонтальный скролл на странице (последний столбец с прочерком очень широкий. зачем там расчет ширины столбцов вообще понадобился?)

Здесь оставил как было, чтобы таблицы визуально почти не отличались. Там таблетки должны быть (сейчас в mvp они не приходят, там проблема с производительностью, скоро должны вернуться). А фиксированная ширина нужна, чтобы ничего не прыгало при скролле. Потому что на каких-то нодах нет таблеток, на других есть, соответсвенно нижний скроллбар будет появляться и исчезать

@artemmufazalov artemmufazalov merged commit d6197d4 into main Oct 27, 2023
4 checks passed
@artemmufazalov artemmufazalov deleted the nodes-virtual-table branch October 27, 2023 14:02
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.

2 participants