-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
AIP-38 | List Import Errors #44021
base: main
Are you sure you want to change the base?
AIP-38 | List Import Errors #44021
Changes from 6 commits
e300e1d
6e36388
cc332eb
a1cdfe1
2d6faf2
5ea9c5e
c0faba9
7cd29d9
79fb2f1
308c865
5266154
a6290ba
3a49bc7
b92c977
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/*! | ||
* 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 { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react"; | ||
import { useState } from "react"; | ||
import { FiChevronRight } from "react-icons/fi"; | ||
|
||
import { DAGImportErrorsModal } from "./DAGImportErrorsModal"; | ||
|
||
export const DAGImportErrors = () => { | ||
const { onClose, onOpen, open } = useDisclosure(); | ||
const [importErrorsCount, setImportErrorsCount] = useState<number>(-1); | ||
|
||
const handleImportErrorsCount = (count: number) => { | ||
setImportErrorsCount(count); | ||
}; | ||
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return ( | ||
<Box alignItems="center" display="flex" gap={2}> | ||
{importErrorsCount > 0 && ( | ||
<Button | ||
alignItems="center" | ||
borderRadius="md" | ||
display="flex" | ||
gap={2} | ||
onClick={onOpen} | ||
variant="outline" | ||
> | ||
<Badge background="red" borderRadius="full" color="white" px={2}> | ||
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{importErrorsCount} | ||
</Badge> | ||
<Box alignItems="center" display="flex" gap={1}> | ||
<Text fontWeight="bold">DAG Import Errors</Text> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dag is used instead of DAG in most parts with renaming done in #43325 . Probably could also be done in this PR for user visible content. Thanks. |
||
<FiChevronRight /> | ||
</Box> | ||
</Button> | ||
)} | ||
<DAGImportErrorsModal | ||
onClose={onClose} | ||
onErrorCountChange={handleImportErrorsCount} | ||
open={open} | ||
/> | ||
</Box> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,131 @@ | ||||||
/*! | ||||||
* 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 { VStack, Heading, Text, Box, Button, HStack } from "@chakra-ui/react"; | ||||||
import { useEffect, useState } from "react"; | ||||||
import { PiFilePy } from "react-icons/pi"; | ||||||
|
||||||
import type { ImportErrorResponse } from "openapi/requests/types.gen"; | ||||||
import Time from "src/components/Time"; | ||||||
import { Accordion, Dialog } from "src/components/ui"; | ||||||
import { useImportErrors } from "src/queries/useDagsImportErrors"; | ||||||
|
||||||
type ImportDAGErrorModalProps = { | ||||||
onClose: () => void; | ||||||
onErrorCountChange: (count: number) => void; | ||||||
open: boolean; | ||||||
}; | ||||||
|
||||||
const PAGE_LIMIT = 5; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing some manual testing. I think we can increase this to 15 |
||||||
|
||||||
export const DAGImportErrorsModal: React.FC<ImportDAGErrorModalProps> = ({ | ||||||
onClose, | ||||||
onErrorCountChange, | ||||||
open, | ||||||
}) => { | ||||||
const [currentPage, setCurrentPage] = useState(0); | ||||||
|
||||||
const { data } = useImportErrors({ | ||||||
limit: PAGE_LIMIT, | ||||||
offset: currentPage * PAGE_LIMIT, | ||||||
}); | ||||||
|
||||||
const importErrors: Array<ImportErrorResponse> = data.import_errors; | ||||||
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
const importErrorsCount: number = data.total_entries || 0; | ||||||
const totalPages = Math.ceil(importErrorsCount / PAGE_LIMIT); | ||||||
|
||||||
useEffect(() => { | ||||||
onErrorCountChange(importErrorsCount); | ||||||
}, [importErrorsCount, onErrorCountChange]); | ||||||
|
||||||
useEffect(() => { | ||||||
if (!open) { | ||||||
setCurrentPage(0); | ||||||
} | ||||||
}, [open]); | ||||||
|
||||||
const handleNextPage = () => { | ||||||
if (currentPage < totalPages - 1) { | ||||||
setCurrentPage((prev) => prev + 1); | ||||||
} | ||||||
}; | ||||||
|
||||||
const handlePreviousPage = () => { | ||||||
if (currentPage > 0) { | ||||||
setCurrentPage((prev) => prev - 1); | ||||||
} | ||||||
}; | ||||||
|
||||||
return ( | ||||||
<Dialog.Root onOpenChange={onClose} open={open} size="xl"> | ||||||
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
<Dialog.Content backdrop> | ||||||
<Dialog.Header> | ||||||
<VStack align="start" gap={4} position="sticky" top={0} zIndex={1}> | ||||||
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
<Heading size="xl">DAG Import Errors</Heading> | ||||||
<Text color="gray.500">{`Page ${currentPage + 1} of ${totalPages}`}</Text> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I made this mistake too. But let's use semantic color tokens, |
||||||
</VStack> | ||||||
</Dialog.Header> | ||||||
|
||||||
<Dialog.CloseTrigger /> | ||||||
|
||||||
<Dialog.Body maxH="500px" overflowY="auto"> | ||||||
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
<Accordion.Root collapsible multiple size="md" variant="enclosed"> | ||||||
{importErrors.map((importError: ImportErrorResponse) => ( | ||||||
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
<Accordion.Item | ||||||
key={importError.import_error_id} | ||||||
value={importError.filename} | ||||||
> | ||||||
<Accordion.ItemTrigger cursor="pointer"> | ||||||
<PiFilePy /> | ||||||
{importError.filename} | ||||||
</Accordion.ItemTrigger> | ||||||
<Accordion.ItemContent> | ||||||
<Text color="gray.500" fontSize="sm" mb={1}> | ||||||
Timestamp : <Time datetime={importError.timestamp} /> | ||||||
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
</Text> | ||||||
<Text color="red.600" fontSize="sm" whiteSpace="pre-wrap"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
More semantic tokens. Docs |
||||||
<code>{importError.stack_trace}</code> | ||||||
</Text> | ||||||
</Accordion.ItemContent> | ||||||
</Accordion.Item> | ||||||
))} | ||||||
</Accordion.Root> | ||||||
</Dialog.Body> | ||||||
|
||||||
<Box p={6}> | ||||||
<HStack justify="space-between"> | ||||||
<Button | ||||||
colorPalette="blue" | ||||||
disabled={currentPage === 0} | ||||||
onClick={handlePreviousPage} | ||||||
> | ||||||
Previous | ||||||
</Button> | ||||||
<Button | ||||||
colorPalette="blue" | ||||||
disabled={currentPage === totalPages - 1} | ||||||
onClick={handleNextPage} | ||||||
> | ||||||
Next | ||||||
</Button> | ||||||
</HStack> | ||||||
</Box> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have a |
||||||
</Dialog.Content> | ||||||
</Dialog.Root> | ||||||
); | ||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/*! | ||
* 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 { Box, Flex, Heading } from "@chakra-ui/react"; | ||
import { FiClipboard } from "react-icons/fi"; | ||
|
||
import { DAGImportErrors } from "./DAGImportErrors"; | ||
|
||
export const Stats = () => ( | ||
<Box> | ||
<Flex color="gray.500" mb={2} mt={2}> | ||
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<FiClipboard /> | ||
<Heading ml={1} size="xs"> | ||
Stats | ||
</Heading> | ||
</Flex> | ||
<DAGImportErrors /> | ||
</Box> | ||
); |
bbovenzi marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
/*! | ||
* 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 { useImportErrorServiceGetImportErrors } from "openapi/queries"; | ||
|
||
const queryOptions = { | ||
refetchOnMount: true, | ||
refetchOnReconnect: false, | ||
refetchOnWindowFocus: false, | ||
staleTime: 5 * 60 * 1000, | ||
}; | ||
|
||
export const useImportErrors = ( | ||
searchParams: { | ||
limit?: number; | ||
offset?: number; | ||
orderBy?: string; | ||
} = {}, | ||
) => { | ||
const { limit = 5, offset, orderBy } = searchParams; | ||
|
||
const { data: errorsData, error: errorsError } = | ||
useImportErrorServiceGetImportErrors( | ||
{ limit, offset, orderBy }, | ||
undefined, | ||
queryOptions, | ||
); | ||
|
||
return { | ||
data: { | ||
import_errors: errorsData?.import_errors ?? [], | ||
total_entries: errorsData?.total_entries ?? 0, | ||
}, | ||
error: errorsError, | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you can create
Health/index.ts
to haveexport { Health } from './Health'
to avoid./Health/Health
and just./Health
. Similar change for Stats.