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

Bug fix for data-sources page #1830

Merged
merged 7 commits into from
May 23, 2024
Merged

Bug fix for data-sources page #1830

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
93a19a6
added fix for failing data-sources page
sumukhswamy May 16, 2024
810cd75
added change for job with the id
sumukhswamy May 16, 2024
80a7583
addressed pr comments
sumukhswamy May 17, 2024
2356b78
added fix for flyput
sumukhswamy May 17, 2024
967457e
added fix for flyout
sumukhswamy May 17, 2024
bbd4fd9
Merge branch 'main' into mds-bugs
sumukhswamy May 21, 2024
6d0aa66
refactored the flyout objects with an interface
sumukhswamy May 21, 2024
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
2 changes: 1 addition & 1 deletion public/components/datasources/components/manage/accelerations/acceleration_table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@

interface AccelerationTableProps {
dataSourceName: string;
cacheLoadingHooks: any;

Check warning on line 47 in public/components/datasources/components/manage/accelerations/acceleration_table.tsx

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected any. Specify a different type
}

interface ModalState {
Expand Down Expand Up @@ -75,7 +75,7 @@
if (operationSuccess) {
handleRefresh();
}
}, [operationSuccess]);

Check warning on line 78 in public/components/datasources/components/manage/accelerations/acceleration_table.tsx

View workflow job for this annotation

GitHub Actions / Lint 

React Hook useEffect has a missing dependency: 'handleRefresh'. Either include it or remove the dependency array

const handleActionClick = (
actionType: ModalState['actionType'],
Expand Down Expand Up @@ -115,7 +115,7 @@
setAccelerations(cachedDataSource.accelerations);
setUpdatedTime(cachedDataSource.lastUpdated);
}
}, []);

Check warning on line 118 in public/components/datasources/components/manage/accelerations/acceleration_table.tsx

View workflow job for this annotation

GitHub Actions / Lint 

React Hook useEffect has missing dependencies: 'accelerationsLoadStatus', 'dataSourceName', and 'startLoadingAccelerations'. Either include them or remove the dependency array

useEffect(() => {
if (accelerationsLoadStatus === DirectQueryLoadingStatus.SUCCESS) {
Expand All @@ -129,14 +129,14 @@
if (accelerationsLoadStatus === DirectQueryLoadingStatus.FAILED) {
setIsRefreshing(false);
}
}, [accelerationsLoadStatus]);

Check warning on line 132 in public/components/datasources/components/manage/accelerations/acceleration_table.tsx

View workflow job for this annotation

GitHub Actions / Lint 

React Hook useEffect has a missing dependency: 'dataSourceName'. Either include it or remove the dependency array

const handleRefresh = useCallback(() => {
if (!isCatalogCacheFetching(accelerationsLoadStatus)) {
setIsRefreshing(true);
startLoadingAccelerations(dataSourceName);
startLoadingAccelerations({ dataSourceName });
}
}, [accelerationsLoadStatus]);

Check warning on line 139 in public/components/datasources/components/manage/accelerations/acceleration_table.tsx

View workflow job for this annotation

GitHub Actions / Lint 

React Hook useCallback has missing dependencies: 'dataSourceName' and 'startLoadingAccelerations'. Either include them or remove the dependency array

const RefreshButton = () => {
return (
Expand Down Expand Up @@ -317,7 +317,7 @@
name: 'Actions',
actions: tableActions,
},
] as Array<EuiTableFieldDataColumnType<any>>;

Check warning on line 320 in public/components/datasources/components/manage/accelerations/acceleration_table.tsx

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected any. Specify a different type

const pagination = {
initialPageSize: 10,
Expand Down
6 changes: 5 additions & 1 deletion ...components/manage/accelerations/create_accelerations_flyout/selectors/source_selector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,14 @@
const loadDataSource = () => {
setLoadingComboBoxes({ ...loadingComboBoxes, dataSource: true });
http
.get(DATACONNECTIONS_BASE + `/dataSourceMDSId=${dataSourceMDSId}`)
.get(
dataSourceMDSId
? `${DATACONNECTIONS_BASE}/dataSourceMDSId=${dataSourceMDSId}`
sumukhswamy marked this conversation as resolved.
Show resolved Hide resolved
sumukhswamy marked this conversation as resolved.
Show resolved Hide resolved
: DATACONNECTIONS_BASE
)
.then((res) => {
const isValidDataSource = res.some(
(connection: any) =>

Check warning on line 84 in public/components/datasources/components/manage/accelerations/create_accelerations_flyout/selectors/source_selector.tsx

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected any. Specify a different type
connection.connector.toUpperCase() === 'S3GLUE' &&
connection.name === selectedDatasource
);
Expand Down Expand Up @@ -141,19 +145,19 @@

useEffect(() => {
loadDataSource();
}, []);

Check warning on line 148 in public/components/datasources/components/manage/accelerations/create_accelerations_flyout/selectors/source_selector.tsx

View workflow job for this annotation

GitHub Actions / Lint 

React Hook useEffect has a missing dependency: 'loadDataSource'. Either include it or remove the dependency array

useEffect(() => {
if (accelerationFormData.dataSource !== '') {
loadDatabases();
}
}, [accelerationFormData.dataSource]);

Check warning on line 154 in public/components/datasources/components/manage/accelerations/create_accelerations_flyout/selectors/source_selector.tsx

View workflow job for this annotation

GitHub Actions / Lint 

React Hook useEffect has a missing dependency: 'loadDatabases'. Either include it or remove the dependency array

useEffect(() => {
if (accelerationFormData.database !== '') {
loadTables();
}
}, [accelerationFormData.database]);

Check warning on line 160 in public/components/datasources/components/manage/accelerations/create_accelerations_flyout/selectors/source_selector.tsx

View workflow job for this annotation

GitHub Actions / Lint 

React Hook useEffect has a missing dependency: 'loadTables'. Either include it or remove the dependency array

return (
<>
Expand Down
12 changes: 3 additions & 9 deletions public/components/datasources/components/manage/accelerations/utils/acceleration_utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,24 +93,18 @@ export const CreateAccelerationFlyoutButton = ({
dataSourceName: string;
renderCreateAccelerationFlyout: (
dataSource: string,
dataSourceMDSId?: string,
databaseName?: string,
tableName?: string,
handleRefresh?: () => void
handleRefresh?: () => void,
dataSourceMDSId?: string
) => void;
handleRefresh: () => void;
}) => {
return (
<>
<EuiButton
onClick={() =>
renderCreateAccelerationFlyout(
dataSourceName,
undefined,
undefined,
undefined,
handleRefresh
)
renderCreateAccelerationFlyout(dataSourceName, undefined, undefined, handleRefresh, '')
}
fill
>
Expand Down
8 changes: 4 additions & 4 deletions ...ts/datasources/components/manage/associated_objects/associated_objects_details_flyout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,10 @@ export const AssociatedObjectsDetailsFlyout = ({
onClick={() =>
renderCreateAccelerationFlyout(
datasourceName,
'',
tableDetail.database,
tableDetail.name,
handleRefresh
handleRefresh,
''
)
}
>
Expand Down Expand Up @@ -198,10 +198,10 @@ export const AssociatedObjectsDetailsFlyout = ({
onClick={() =>
renderCreateAccelerationFlyout(
datasourceName,
'',
tableDetail.database,
tableDetail.name,
handleRefresh
handleRefresh,
''
)
}
iconType="popout"
Expand Down
8 changes: 4 additions & 4 deletions ...ic/components/datasources/components/manage/associated_objects/associated_objects_tab.tsx
View file
Open in desktop
Copy link
Contributor

@amsiglan amsiglan May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not a blocker: The type for cacheLoadingHooks: any; or it's properties shouldn't be any, it does not tell you the signature of the parameters you should be passing and hence we see such major bugs creeping in.
Can we please add correct type annotations here?

Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export const AssociatedObjectsTab: React.FC<AssociatedObjectsTabProps> = (props)

const onRefreshButtonClick = () => {
if (!isCatalogCacheFetching(databasesLoadStatus, tablesLoadStatus, accelerationsLoadStatus)) {
startLoadingDatabases({ databaseName: datasource.name });
startLoadingDatabases({ dataSourceName: datasource.name });
setIsRefreshing(true);
}
};
Expand Down Expand Up @@ -167,7 +167,7 @@ export const AssociatedObjectsTab: React.FC<AssociatedObjectsTabProps> = (props)
datasourceCache.status === CachedDataSourceStatus.Failed) &&
!isCatalogCacheFetching(databasesLoadStatus)
) {
startLoadingDatabases(datasource.name);
startLoadingDatabases({ dataSourceName: datasource.name });
} else if (datasourceCache.status === CachedDataSourceStatus.Updated) {
setCachedDatabases(datasourceCache.databases);
setIsFirstTimeLoading(false);
Expand Down Expand Up @@ -224,7 +224,7 @@ export const AssociatedObjectsTab: React.FC<AssociatedObjectsTabProps> = (props)
databaseCache.status === CachedDataSourceStatus.Failed) &&
!isCatalogCacheFetching(tablesLoadStatus)
) {
startLoadingTables(datasource.name, selectedDatabase);
startLoadingTables({ dataSourceName: datasource.name, databaseName: selectedDatabase });
setIsObjectsLoading(true);
} else if (databaseCache.status === CachedDataSourceStatus.Updated) {
setCachedTables(databaseCache.tables);
Expand All @@ -235,7 +235,7 @@ export const AssociatedObjectsTab: React.FC<AssociatedObjectsTabProps> = (props)
isRefreshing) &&
!isCatalogCacheFetching(accelerationsLoadStatus)
) {
startLoadingAccelerations(datasource.name);
startLoadingAccelerations({ dataSourceName: datasource.name });
setIsObjectsLoading(true);
} else if (accelerationsCache.status === CachedDataSourceStatus.Updated) {
setCachedAccelerations(accelerationsCache.accelerations);
Expand Down
1 change: 0 additions & 1 deletion public/components/hooks/use_polling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ export function usePolling<T, P = void>(
try {
const result = await fetchFunction(params);
setData(result);
console.log(result);
// Check the success condition and stop polling if it's met
if (onPollingSuccess && onPollingSuccess(result, configurations)) {
stopPolling();
Expand Down
1 change: 0 additions & 1 deletion public/framework/catalog_cache/cache_loader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,6 @@ export const useLoadToCache = (loadCacheType: LoadCacheType) => {
sqlService
.fetch(requestPayload, dataSourceMDSId)
.then((result) => {
console.log(result);
setAsyncSessionId(dataSourceName, getObjValue(result, 'sessionId', null));
if (result.queryId) {
startPolling({
Expand Down
24 changes: 12 additions & 12 deletions public/plugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
* SPDX-License-Identifier: Apache-2.0
*/

import React from 'react';
import { i18n } from '@osd/i18n';
import { htmlIdGenerator } from '@elastic/eui';
import { i18n } from '@osd/i18n';
import React from 'react';
sumukhswamy marked this conversation as resolved.
Show resolved Hide resolved
import {
AppCategory,
AppMountParameters,
Expand All @@ -18,6 +18,12 @@ import {
} from '../../../src/core/public';
import { toMountPoint } from '../../../src/plugins/opensearch_dashboards_react/public/';
import { createGetterSetter } from '../../../src/plugins/opensearch_dashboards_utils/public';
import {
DATA_SOURCE_TYPES,
OBS_S3_DATA_SOURCE,
S3_DATA_SOURCE_GROUP_DISPLAY_NAME,
S3_DATA_SOURCE_GROUP_SPARK_DISPLAY_NAME,
} from '../common/constants/data_sources';
import { CREATE_TAB_PARAM, CREATE_TAB_PARAM_KEY, TAB_CHART_ID } from '../common/constants/explorer';
import {
DATACONNECTIONS_BASE,
Expand Down Expand Up @@ -96,12 +102,6 @@ import {
ObservabilityStart,
SetupDependencies,
} from './types';
import {
DATA_SOURCE_TYPES,
OBS_S3_DATA_SOURCE,
S3_DATA_SOURCE_GROUP_DISPLAY_NAME,
S3_DATA_SOURCE_GROUP_SPARK_DISPLAY_NAME,
} from '../common/constants/data_sources';

interface PublicConfig {
query_assist: {
Expand Down Expand Up @@ -142,10 +142,10 @@ export const [
] = createGetterSetter<
(
dataSource: string,
dataSourceMDSId?: string,
databaseName?: string,
tableName?: string,
handleRefresh?: () => void
handleRefresh?: () => void,
dataSourceMDSId?: string
) => void
>('renderCreateAccelerationFlyout');

Expand Down Expand Up @@ -515,10 +515,10 @@ export class ObservabilityPlugin

const renderCreateAccelerationFlyout = (
selectedDatasource: string,
dataSourceMDSId?: string,
databaseName?: string,
tableName?: string,
handleRefresh?: () => void
handleRefresh?: () => void,
dataSourceMDSId?: string
Copy link
Member

Choose a reason for hiding this comment

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

This order change might need a change in workbench? How do we know this is not causing a new regression, do we have test setup for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes same instance with the fix has it

Copy link
Member

Choose a reason for hiding this comment

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

I meant some cypress tests or jest tests, to catch any regression in workbench. For example in workbench we are passing the params to this function: https://github.com/opensearch-project/dashboards-query-workbench/blob/888a8fe83b5e0a732866aaddb7a229eabed0f150/public/components/SQLPage/SQLPage.tsx#L71 Since, we do not have named params the MDSId will be mapped to databaseName now.

) => {
const createAccelerationFlyout = core.overlays.openFlyout(
toMountPoint(
Expand Down
2 changes: 1 addition & 1 deletion public/services/requests/sql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export class SQLService {
errorHandler?: (error: any) => void
) => {
return this.http
.get(`/api/observability/query/jobs/${params.queryId}/${dataSourceMDSId}`)
.get(`/api/observability/query/jobs/${params.queryId}/${dataSourceMDSId ?? ''}`)
.catch((error) => {
console.error('fetch error: ', error.body);
if (errorHandler) errorHandler(error);
Expand Down
4 changes: 2 additions & 2 deletions public/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ export interface ObservabilityStart {
) => void;
renderCreateAccelerationFlyout: (
dataSource: string,
dataSourceMDSId?: string,
databaseName?: string,
tableName?: string,
handleRefresh?: () => void
handleRefresh?: () => void,
dataSourceMDSId?: string
) => void;
CatalogCacheManagerInstance: typeof CatalogCacheManager;
useLoadDatabasesToCacheHook: () => LoadCachehookOutput;
Expand Down
Loading