Skip to content

[DataViews] Fix checking remote clusters in empty state #110054

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

Merged
Dosant merged 3 commits into from
Aug 26, 2021
Merged

[DataViews] Fix checking remote clusters in empty state #110054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
213ac6a
fix checking remote cluster for empty state
Dosant Aug 25, 2021
927bdee
remote redundant effect
Dosant Aug 25, 2021
6875374
remove redundant isMounted checks to avoid potential bugs
Dosant Aug 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions src/plugins/index_pattern_editor/public/components/empty_prompts/empty_prompts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import React, { useState, useCallback, FC } from 'react';
import React, { useState, FC, useEffect } from 'react';
import useAsync from 'react-use/lib/useAsync';

import { useKibana } from '../../shared_imports';
Expand Down Expand Up @@ -47,31 +47,30 @@ export const EmptyPrompts: FC<Props> = ({ allSources, onCancel, children, loadSo
} = useKibana<IndexPatternEditorContext>();

const [remoteClustersExist, setRemoteClustersExist] = useState<boolean>(false);
const [hasCheckedRemoteClusters, setHasCheckedRemoteClusters] = useState<boolean>(false);

const [goToForm, setGoToForm] = useState<boolean>(false);

const hasDataIndices = allSources.some(isUserDataIndex);
const hasUserIndexPattern = useAsync(() =>
indexPatternService.hasUserIndexPattern().catch(() => true)
);

useCallback(() => {
let isMounted = true;
if (!hasDataIndices)
useEffect(() => {
Copy link
Copy Markdown
Contributor Author

@Dosant Dosant Aug 25, 2021

Choose a reason for hiding this comment

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

This is the main fix [DataViews] Fix checking remote clusters in empty state

We had useCallback instead of useEffect by mistake

if (!hasDataIndices && !hasCheckedRemoteClusters) {
setHasCheckedRemoteClusters(true);

getIndices({
http,
isRollupIndex: () => false,
pattern: '*:*',
showAllIndices: false,
searchClient,
}).then((dataSources) => {
if (isMounted) {
setRemoteClustersExist(!!dataSources.filter(removeAliases).length);
}
setRemoteClustersExist(!!dataSources.filter(removeAliases).length);
});
return () => {
isMounted = false;
};
}, [http, hasDataIndices, searchClient]);
}
}, [http, hasDataIndices, searchClient, hasCheckedRemoteClusters]);

if (hasUserIndexPattern.loading) return null; // return null to prevent UI flickering while loading

Expand Down
32 changes: 7 additions & 25 deletions src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ const IndexPatternEditorFlyoutContentComponent = ({
defaultTypeIsRollup,
requireTimestampField = false,
}: Props) => {
const isMounted = useRef<boolean>(false);
const {
services: { http, indexPatternService, uiSettings, searchClient },
} = useKibana<IndexPatternEditorContext>();
Expand Down Expand Up @@ -156,30 +155,23 @@ const IndexPatternEditorFlyoutContentComponent = ({

// loading list of index patterns
useEffect(() => {
isMounted.current = true;
Copy link
Copy Markdown
Contributor Author

@Dosant Dosant Aug 25, 2021

Choose a reason for hiding this comment

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

(not related to the main fix, but way too minor to extract to separate pr)

I noticed that how this isMounted was setup is potentially risky because it was set up inside an effect with particular dependencies and then reused in other places which are likely dependent on other things.

Instead of fixing it, I removed it. (no need for it because no race condition or potential memory leaks here) see: https://github.com/facebook/react/pull/22114

loadSources();
const getTitles = async () => {
const indexPatternTitles = await indexPatternService.getTitles();
if (isMounted.current) {
setExistingIndexPatterns(indexPatternTitles);
setIsLoadingIndexPatterns(false);
}

setExistingIndexPatterns(indexPatternTitles);
setIsLoadingIndexPatterns(false);
};
getTitles();
return () => {
isMounted.current = false;
};
}, [http, indexPatternService, loadSources]);

// loading rollup info
useEffect(() => {
const getRollups = async () => {
try {
const response = await http.get('/api/rollup/indices');
if (isMounted.current) {
if (response) {
setRollupIndicesCapabilities(response);
}
if (response) {
setRollupIndicesCapabilities(response);
}
} catch (e) {
// Silently swallow failure responses such as expired trials
Expand Down Expand Up @@ -214,10 +206,7 @@ const IndexPatternEditorFlyoutContentComponent = ({
);
timestampOptions = extractTimeFields(fields, requireTimestampField);
}
if (
isMounted.current &&
currentLoadingTimestampFieldsIdx === currentLoadingTimestampFieldsRef.current
) {
if (currentLoadingTimestampFieldsIdx === currentLoadingTimestampFieldsRef.current) {
setIsLoadingTimestampFields(false);
setTimestampFieldOptions(timestampOptions);
}
Expand Down Expand Up @@ -266,10 +255,7 @@ const IndexPatternEditorFlyoutContentComponent = ({
exactMatched: [],
};

if (
currentLoadingMatchedIndicesIdx === currentLoadingMatchedIndicesRef.current &&
isMounted.current
) {
if (currentLoadingMatchedIndicesIdx === currentLoadingMatchedIndicesRef.current) {
// we are still interested in this result
if (type === INDEX_PATTERN_TYPE.ROLLUP) {
const rollupIndices = exactMatched.filter((index) => isRollupIndex(index.name));
Expand All @@ -291,10 +277,6 @@ const IndexPatternEditorFlyoutContentComponent = ({
[http, allowHidden, allSources, type, rollupIndicesCapabilities, searchClient, isLoadingSources]
);

useEffect(() => {
reloadMatchedIndices(title);
Copy link
Copy Markdown
Contributor Author

@Dosant Dosant Aug 25, 2021

Choose a reason for hiding this comment

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

@sebelga pointed out that this might be redundant: #109500 (comment)
I tried to remove and indeed don't see any regressions

(not related to the main fix, but way too minor to extract to separate pr)

Copy link
Copy Markdown
Contributor

@sebelga sebelga Aug 26, 2021

Choose a reason for hiding this comment

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

👍 I'd expect the number of requests to go from 3 to 2 (without the memoizeOnce you added).

}, [allowHidden, reloadMatchedIndices, title]);

const onTypeChange = useCallback(
(newType) => {
form.setFieldValue('title', '');
Expand Down