aminghadersohi commented on code in PR #40512: URL: https://github.com/apache/superset/pull/40512#discussion_r3481447346
########## docs/docs/faq.mdx: ########## @@ -181,6 +181,20 @@ value in milliseconds in the JSON Metadata field: Here, the entire dashboard will refresh at once if periodic refresh is on. The stagger time of 2.5 seconds is ignored. +The manual **Refresh dashboard** button can also stagger its chart requests, controlled by the +`SUPERSET_DASHBOARD_MANUAL_REFRESH_STAGGER_MS` server config in `superset_config.py`. This defaults +to `0`, which preserves the original behavior where every chart request fires at the same time when +the button is clicked. To opt in to staggering, set a positive number of milliseconds; the window Review Comment: MEDIUM: This paragraph doesn’t mention that the per-dashboard JSON metadata flag (shown in the example at the top of this FAQ section, lines 175–178) also suppresses staggering on the manual-refresh path. In , when is , is forced to regardless of — so a positive has no effect on dashboards where that flag is set. An operator who enables the new config but has dashboards with will see no staggering and have no diagnostic hint. A one-sentence note here (e.g. “Note: the per-dashboard override also disables manual-refresh staggering”) would close the gap. ########## superset/config.py: ########## @@ -215,6 +215,17 @@ def _try_json_readsha(filepath: str, length: int) -> str | None: SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT = 0 SUPERSET_DASHBOARD_PERIODICAL_REFRESH_WARNING_MESSAGE = None +# Manual dashboard refresh can stagger chart data requests across this many +# milliseconds so they do not all hit the backend at the same instant. This +# defaults to 0, which preserves the original behavior where every chart +# request fires at the same time when the user clicks the Refresh dashboard +# button. Set a positive value to opt in to staggering; the frontend then +# uses the larger of this value and the dashboard's stagger_time metadata. Review Comment: NIT: The comment says “the frontend then uses the larger of this value and the dashboard’s metadata” but omits that defaults to ms when not set ( in ). An operator who sets will silently get a 5 s window on dashboards that don’t configure explicitly. Appending “which defaults to 5000 ms when not explicitly set in dashboard metadata” would prevent that surprise. ########## superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.test.tsx: ########## @@ -0,0 +1,154 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { renderHook, act } from '@testing-library/react'; +import { Provider } from 'react-redux'; +import { createStore } from 'redux'; +import { ReactNode } from 'react'; +import { LOG_ACTIONS_FORCE_REFRESH_DASHBOARD } from 'src/logger/LogUtils'; +import { useHeaderAutoRefresh } from './useHeaderAutoRefresh'; + +jest.mock('src/dashboard/contexts/AutoRefreshContext', () => ({ + useAutoRefreshContext: () => ({ + startAutoRefresh: jest.fn(), + endAutoRefresh: jest.fn(), + setRefreshInFlight: jest.fn(), + }), +})); + +jest.mock('src/dashboard/hooks/useRealTimeDashboard', () => ({ + useRealTimeDashboard: () => ({ + isPaused: false, + setStatus: jest.fn(), + setPaused: jest.fn(), + setPausedByTab: jest.fn(), + recordSuccess: jest.fn(), + recordError: jest.fn(), + setFetchStartTime: jest.fn(), + autoRefreshPauseOnInactiveTab: false, + setPauseOnInactiveTab: jest.fn(), + }), +})); + +jest.mock('src/dashboard/hooks/useAutoRefreshTabPause', () => ({ + useAutoRefreshTabPause: jest.fn(), +})); + +const createWrapper = (conf: Record<string, unknown> = {}) => { + const store = createStore(() => ({ + charts: { + 1: { latestQueryFormData: { datasource: '1__table' } }, + 2: { latestQueryFormData: { datasource: '2__table' } }, + }, + dashboardInfo: { + common: { conf }, + }, + })); + return ({ children }: { children: ReactNode }) => ( + <Provider store={store}>{children}</Provider> + ); +}; + +const renderHeaderAutoRefresh = ( + conf: Record<string, unknown> = {}, + overrides = {}, +) => { + const props = { + chartIds: [1, 2], + dashboardId: 100, + refreshFrequency: 0, + timedRefreshImmuneSlices: [], + isLoading: false, + onRefresh: jest.fn().mockResolvedValue(undefined), + setRefreshFrequency: jest.fn(), + logEvent: jest.fn(), + ...overrides, + }; + const { result } = renderHook(() => useHeaderAutoRefresh(props), { + wrapper: createWrapper(conf), + }); + return { result, props }; +}; + +test('forceRefresh passes the default stagger interval (5000ms) when no config is provided', async () => { Review Comment: MEDIUM: These four tests only verify that is called with the right (the config-selection layer in the hook). The actual stagger dispatch in — — is never exercised because is mocked at the hook boundary. Concretely: removing the -based stagger loop from entirely would leave all four tests green. A test that calls through to the real dispatch, advances time by increments, and asserts each fires at the expected interval would pin the core anti-thundering-herd mechanic that this PR introduces. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
