pierrejeambrun commented on code in PR #49794: URL: https://github.com/apache/airflow/pull/49794#discussion_r2063608208
########## airflow-core/src/airflow/ui/src/pages/Dashboard/PoolSummary/PoolSummary.tsx: ########## @@ -0,0 +1,113 @@ +/*! + * 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, Heading, Flex, Skeleton } from "@chakra-ui/react"; +import { BiTargetLock } from "react-icons/bi"; + +import { usePoolServiceGetPools } from "openapi/queries/queries"; +import { Tooltip } from "src/components/ui"; +import { capitalize } from "src/utils"; +import { slots } from "src/utils/slots"; + +export const PoolSummary = () => { + const { data, isLoading } = usePoolServiceGetPools(); + const pools = data?.pools ?? []; + const totalSlots = pools.reduce((sum, pool) => sum + pool.slots, 0); + + const slotTotals = { + deferred_slots: 0, + occupied_slots: 0, + open_slots: 0, + queued_slots: 0, + running_slots: 0, + scheduled_slots: 0, + }; + + const poolsWithSlotType = { + deferred_slots: 0, + occupied_slots: 0, + open_slots: 0, + queued_slots: 0, + running_slots: 0, + scheduled_slots: 0, + }; + + pools.forEach((pool) => { + Object.keys(slotTotals).forEach((slotKey) => { + const typedKey = slotKey as keyof typeof slotTotals; Review Comment: That's not great, we souldn't use that with proper typing. ########## airflow-core/src/airflow/ui/src/pages/Dashboard/PoolSummary/PoolSummary.tsx: ########## @@ -0,0 +1,113 @@ +/*! + * 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, Heading, Flex, Skeleton } from "@chakra-ui/react"; +import { BiTargetLock } from "react-icons/bi"; + +import { usePoolServiceGetPools } from "openapi/queries/queries"; +import { Tooltip } from "src/components/ui"; +import { capitalize } from "src/utils"; +import { slots } from "src/utils/slots"; + +export const PoolSummary = () => { + const { data, isLoading } = usePoolServiceGetPools(); + const pools = data?.pools ?? []; + const totalSlots = pools.reduce((sum, pool) => sum + pool.slots, 0); + + const slotTotals = { + deferred_slots: 0, + occupied_slots: 0, + open_slots: 0, + queued_slots: 0, + running_slots: 0, + scheduled_slots: 0, + }; + + const poolsWithSlotType = { + deferred_slots: 0, + occupied_slots: 0, + open_slots: 0, + queued_slots: 0, + running_slots: 0, + scheduled_slots: 0, + }; + + pools.forEach((pool) => { + Object.keys(slotTotals).forEach((slotKey) => { + const typedKey = slotKey as keyof typeof slotTotals; + const slotValue = pool[typedKey]; + + if (typeof slotValue === "number" && slotValue > 0) { + slotTotals[typedKey] += slotValue; + poolsWithSlotType[typedKey] += 1; + } + }); + }); + + return ( + <Box w="100%"> + <Flex color="fg.muted" mb={2} w="100%"> + <BiTargetLock /> + <Heading ml={1} size="xs"> + Pool Slots + </Heading> + </Flex> + + {isLoading ? ( + <Skeleton borderRadius="full" h={8} w="100%" /> + ) : ( + <Box> + <Flex bg="white" borderRadius="full" overflow="hidden" w="100%"> + {Object.entries(slots).map(([slotKey, { color, icon }]) => { + const typedKey = slotKey as keyof typeof slotTotals; + const slotValue = slotTotals[typedKey]; + const flexValue = slotValue / totalSlots || 0; + + if (flexValue === 0) { + return undefined; + } + + const tooltipContent = `${capitalize(slotKey.replace("_", " "))}: ${slotValue} ${ + poolsWithSlotType[typedKey] > 1 ? `(${poolsWithSlotType[typedKey]} pools)` : "" + }`; + + return ( + <Tooltip content={tooltipContent} key={slotKey}> + <Flex + alignItems="center" + bg={`${color}.solid`} + color="white" + flex={flexValue} + gap={1} + h="100%" + justifyContent="center" + py={0.5} + textAlign="center" + > + {icon} + {slotValue} + </Flex> + </Tooltip> + ); + })} + </Flex> Review Comment: I think we should refactor and reuse here the `PoolBar` component. (make it a shared component. It's weird to have a slightly different design between the two. (border radius, icons vs number etc...) ########## airflow-core/src/airflow/ui/src/pages/Dashboard/PoolSummary/PoolSummary.tsx: ########## @@ -0,0 +1,113 @@ +/*! + * 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, Heading, Flex, Skeleton } from "@chakra-ui/react"; +import { BiTargetLock } from "react-icons/bi"; + +import { usePoolServiceGetPools } from "openapi/queries/queries"; +import { Tooltip } from "src/components/ui"; +import { capitalize } from "src/utils"; +import { slots } from "src/utils/slots"; + +export const PoolSummary = () => { + const { data, isLoading } = usePoolServiceGetPools(); + const pools = data?.pools ?? []; + const totalSlots = pools.reduce((sum, pool) => sum + pool.slots, 0); + + const slotTotals = { + deferred_slots: 0, + occupied_slots: 0, + open_slots: 0, + queued_slots: 0, + running_slots: 0, + scheduled_slots: 0, + }; + + const poolsWithSlotType = { + deferred_slots: 0, + occupied_slots: 0, + open_slots: 0, + queued_slots: 0, + running_slots: 0, + scheduled_slots: 0, + }; + + pools.forEach((pool) => { + Object.keys(slotTotals).forEach((slotKey) => { + const typedKey = slotKey as keyof typeof slotTotals; + const slotValue = pool[typedKey]; + + if (typeof slotValue === "number" && slotValue > 0) { Review Comment: Why this check ? Return type is `number` for all of those fields. ########## airflow-core/src/airflow/ui/src/pages/Dashboard/PoolSummary/PoolSummary.tsx: ########## @@ -0,0 +1,113 @@ +/*! + * 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, Heading, Flex, Skeleton } from "@chakra-ui/react"; +import { BiTargetLock } from "react-icons/bi"; + +import { usePoolServiceGetPools } from "openapi/queries/queries"; +import { Tooltip } from "src/components/ui"; +import { capitalize } from "src/utils"; +import { slots } from "src/utils/slots"; + +export const PoolSummary = () => { + const { data, isLoading } = usePoolServiceGetPools(); Review Comment: Can we add some auto refresh there? ########## airflow-core/src/airflow/ui/src/pages/Dashboard/PoolSummary/PoolSummary.tsx: ########## @@ -0,0 +1,113 @@ +/*! + * 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, Heading, Flex, Skeleton } from "@chakra-ui/react"; +import { BiTargetLock } from "react-icons/bi"; + +import { usePoolServiceGetPools } from "openapi/queries/queries"; +import { Tooltip } from "src/components/ui"; +import { capitalize } from "src/utils"; +import { slots } from "src/utils/slots"; + +export const PoolSummary = () => { + const { data, isLoading } = usePoolServiceGetPools(); + const pools = data?.pools ?? []; Review Comment: That's not great there's other way to initialize data before the first fetch with react query. (while data is undefined) ########## airflow-core/src/airflow/ui/src/pages/Dashboard/Dashboard.tsx: ########## @@ -61,8 +62,9 @@ export const Dashboard = () => { <Box> <Stats /> </Box> - <Box mt={8}> + <Box display="flex" gap={8} mt={8} width="full"> Review Comment: ```suggestion <Box display="flex" gap={8} mt={8}> ``` -- 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]
