From 0981b8b0f877b9c3a848d7a148ce08dafbc1587a Mon Sep 17 00:00:00 2001 From: Mathieu Anderson Date: Thu, 23 Nov 2023 16:34:24 +0100 Subject: [PATCH 1/2] feat(coral): Do not eagerly fetch Consumer offsets, render all partitions Signed-off-by: Mathieu Anderson --- .../components/ConsumerOffsetsValues.test.tsx | 133 ++++++++++++++++++ .../components/ConsumerOffsetsValues.tsx | 125 ++++++++++++++++ .../TopicSubscriptionsDetailsModal.test.tsx | 62 ++++---- .../TopicSubscriptionsDetailsModal.tsx | 73 +++------- 4 files changed, 318 insertions(+), 75 deletions(-) create mode 100644 coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.test.tsx create mode 100644 coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.tsx diff --git a/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.test.tsx b/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.test.tsx new file mode 100644 index 0000000000..71f21366a7 --- /dev/null +++ b/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.test.tsx @@ -0,0 +1,133 @@ +import { cleanup, screen } from "@testing-library/react"; +import { userEvent } from "@testing-library/user-event"; +import { ConsumerOffsetsValues } from "src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues"; +import { getConsumerOffsets } from "src/domain/acl/acl-api"; +import { ConsumerOffsets } from "src/domain/acl/acl-types"; +import { customRender } from "src/services/test-utils/render-with-wrappers"; + +jest.mock("src/domain/acl/acl-api"); + +const mockGetConsumerOffsets = getConsumerOffsets as jest.MockedFunction< + typeof getConsumerOffsets +>; + +const testOffsetsDataOnePartition: ConsumerOffsets[] = [ + { + topicPartitionId: "0", + currentOffset: "0", + endOffset: "0", + lag: "0", + }, +]; + +const testOffsetsDataTwoPartitions: ConsumerOffsets[] = [ + { + topicPartitionId: "0", + currentOffset: "0", + endOffset: "0", + lag: "0", + }, + { + topicPartitionId: "1", + currentOffset: "0", + endOffset: "0", + lag: "0", + }, +]; + +const testOffsetsNoData: ConsumerOffsets[] = []; + +const props = { + topicName: "aivtopic3", + consumerGroup: "-na-", + environment: "1", + setError: jest.fn(), +}; + +describe("ConsumerOffsetValues.tsx", () => { + beforeEach(() => { + customRender(, { + queryClient: true, + }); + }); + afterEach(() => { + jest.resetAllMocks(); + cleanup(); + }); + + it("does not call getConsumerOffsets on load", () => { + expect(mockGetConsumerOffsets).not.toHaveBeenCalled(); + }); + it("renders correct initial state", () => { + const fetchButton = screen.getByRole("button", { + name: "Fetch the consumer offsets of the current subscription", + }); + const offsetsText = screen.getByText("Fetch offsets to display data."); + + expect(fetchButton).toBeEnabled(); + expect(offsetsText).toBeVisible(); + }); + it("renders Consumer offsets when clicking Fetch offsets button (one partition)", async () => { + mockGetConsumerOffsets.mockResolvedValue(testOffsetsDataOnePartition); + + const fetchButton = screen.getByRole("button", { + name: "Fetch the consumer offsets of the current subscription", + }); + + await userEvent.click(fetchButton); + + const offsets = screen.getByText( + "Partition 0: Current offset 0 | End offset 0 | Lag 0" + ); + expect(offsets).toBeVisible(); + + const refetchButton = screen.getByRole("button", { + name: "Refetch the consumer offsets of the current subscription", + }); + + expect(refetchButton).toBeEnabled(); + }); + it("renders Consumer offsets when clicking Fetch offsets button (two partitions)", async () => { + mockGetConsumerOffsets.mockResolvedValue(testOffsetsDataTwoPartitions); + + const fetchButton = screen.getByRole("button", { + name: "Fetch the consumer offsets of the current subscription", + }); + + await userEvent.click(fetchButton); + + const offsetsPartitionOne = screen.getByText( + "Partition 0: Current offset 0 | End offset 0 | Lag 0" + ); + const offsetsPartitionTwo = screen.getByText( + "Partition 1: Current offset 0 | End offset 0 | Lag 0" + ); + expect(offsetsPartitionOne).toBeVisible(); + expect(offsetsPartitionTwo).toBeVisible(); + + const refetchButton = screen.getByRole("button", { + name: "Refetch the consumer offsets of the current subscription", + }); + + expect(refetchButton).toBeEnabled(); + }); + it("renders no data message when clicking Fetch offsets button (no data)", async () => { + mockGetConsumerOffsets.mockResolvedValue(testOffsetsNoData); + + const fetchButton = screen.getByRole("button", { + name: "Fetch the consumer offsets of the current subscription", + }); + + await userEvent.click(fetchButton); + + const noOffsets = screen.getByText("No offsets are currently retained."); + + expect(noOffsets).toBeVisible(); + + const refetchButton = screen.getByRole("button", { + name: "Refetch the consumer offsets of the current subscription", + }); + + expect(refetchButton).toBeEnabled(); + }); +}); diff --git a/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.tsx b/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.tsx new file mode 100644 index 0000000000..02b1ae03d6 --- /dev/null +++ b/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.tsx @@ -0,0 +1,125 @@ +import { Box, Button, Grid, Skeleton, Typography } from "@aivenio/aquarium"; +import refreshIcon from "@aivenio/aquarium/dist/src/icons/refresh"; +import { useQuery } from "@tanstack/react-query"; +import { useEffect, useState } from "react"; +import { getConsumerOffsets } from "src/domain/acl/acl-api"; +import { ConsumerOffsets } from "src/domain/acl/acl-types"; +import { HTTPError } from "src/services/api"; +import { parseErrorMsg } from "src/services/mutation-utils"; + +interface ConsumerOffsetsProps { + setError: (error: string) => void; + topicName: string; + environment: string; + consumerGroup?: string; +} + +const parseOffsetsContent = ({ + topicPartitionId = "Unknown", + currentOffset = "Unknown", + endOffset = "Unknown", + lag = "Unknown", +}: ConsumerOffsets) => { + return `Partition ${topicPartitionId}: Current offset ${currentOffset} | End offset ${endOffset} | Lag ${lag}`; +}; + +const ConsumerOffsetsValues = ({ + setError, + topicName, + environment, + consumerGroup, +}: ConsumerOffsetsProps) => { + const [shouldFetch, setShouldFetch] = useState(false); + const { + data: offsetsData = [], + error: offsetsError, + isFetched: offsetsDataFetched, + isFetching, + refetch, + } = useQuery( + ["getConsumerOffsets", topicName, environment, consumerGroup], + { + queryFn: () => { + return getConsumerOffsets({ + topicName, + env: environment, + consumerGroupId: consumerGroup || "", + }); + }, + enabled: shouldFetch, + } + ); + + useEffect(() => { + if (offsetsError !== null) { + setError(parseErrorMsg(offsetsError)); + } + }, [offsetsError]); + + return ( + + + {!shouldFetch && ( + + Fetch offsets to display data. + + )} + + {offsetsData.length === 0 && offsetsDataFetched && ( + + No offsets are currently retained. + + )} + {isFetching ? ( + + {/* Render one skeleton on first fetch + Render as many skeletons as partitions for refetch */} + {(offsetsData.length === 0 ? ["skeleton"] : offsetsData).map( + (_, index) => { + return ; + } + )} + + ) : ( + offsetsData.map((data, index) => { + return ( + + {parseOffsetsContent(data)} + + ); + }) + )} + + + + { + if (!shouldFetch) { + setShouldFetch(true); + return; + } + refetch(); + }} + disabled={isFetching} + loading={isFetching} + aria-label={`${ + shouldFetch ? "Refetch" : "Fetch" + } the consumer offsets of the current subscription`} + icon={refreshIcon} + > + {shouldFetch ? "Refetch" : "Fetch"} offsets + + + + ); +}; + +export { ConsumerOffsetsValues }; diff --git a/coral/src/app/features/topics/details/subscriptions/components/TopicSubscriptionsDetailsModal.test.tsx b/coral/src/app/features/topics/details/subscriptions/components/TopicSubscriptionsDetailsModal.test.tsx index fca754587d..6ddda63d46 100644 --- a/coral/src/app/features/topics/details/subscriptions/components/TopicSubscriptionsDetailsModal.test.tsx +++ b/coral/src/app/features/topics/details/subscriptions/components/TopicSubscriptionsDetailsModal.test.tsx @@ -3,6 +3,7 @@ import { screen, waitForElementToBeRemoved, } from "@testing-library/react"; +import { userEvent } from "@testing-library/user-event"; import TopicSubscriptionsDetailsModal from "src/app/features/topics/details/subscriptions/components/TopicSubscriptionsDetailsModal"; import { getAivenServiceAccountDetails, @@ -33,12 +34,14 @@ const notOwnerTestServiceAccountData = { accountFound: false, }; -const testOffsetsData = { - topicPartitionId: "0", - currentOffset: "0", - endOffset: "0", - lag: "0", -}; +const testOffsetsData = [ + { + topicPartitionId: "0", + currentOffset: "0", + endOffset: "0", + lag: "0", + }, +]; const testSelectedSubAiven: AclOverviewInfo = { req_no: "1006", @@ -133,7 +136,7 @@ describe("TopicSubscriptionsDetailsModal.tsx", () => { cleanup(); }); - it("does not fetch data for consumer offset", async () => { + it("does not fetch data for Consumer offsets on load", async () => { expect(mockGetConsumerOffsets).not.toHaveBeenCalled(); }); @@ -189,13 +192,13 @@ describe("TopicSubscriptionsDetailsModal.tsx", () => { findDefinition(defaultPropsAiven.serviceAccountData.password) ).toBeVisible(); - expect(screen.queryByText("Consumer offset")).not.toBeInTheDocument(); + expect(screen.queryByText("Consumer offsets")).not.toBeInTheDocument(); }); }); describe("renders correct data in details modal (non Aiven consumer)", () => { beforeEach(() => { - mockGetConsumerOffsets.mockResolvedValue([testOffsetsData]); + mockGetConsumerOffsets.mockResolvedValue(testOffsetsData); customRender( , @@ -209,22 +212,15 @@ describe("TopicSubscriptionsDetailsModal.tsx", () => { cleanup(); }); - it("fetches data for consumer offset", async () => { - expect(mockGetConsumerOffsets).toHaveBeenCalledWith({ - consumerGroupId: - defaultPropsNonAiven.selectedSubscription.consumergroup, - env: defaultPropsNonAiven.selectedSubscription.environment, - topicName: defaultPropsNonAiven.selectedSubscription.topicname, - }); + it("does not fetch data for Consumer offsets on load", async () => { + expect(mockGetConsumerOffsets).not.toHaveBeenCalled(); }); it("does not fetch service account details from aiven", async () => { expect(mockGetAivenServiceAccountDetails).not.toHaveBeenCalled(); }); - it("renders correct data in details modal (non Aiven consumer)", async () => { - await waitForElementToBeRemoved(screen.getByTestId("offsets-skeleton")); - + it("renders correct initial data in details modal (non Aiven consumer)", async () => { expect(findTerm("Environment")).toBeVisible(); expect( findDefinition( @@ -262,16 +258,32 @@ describe("TopicSubscriptionsDetailsModal.tsx", () => { findDefinition(defaultPropsNonAiven.selectedSubscription.acl_ip) ).toBeVisible(); - expect( - screen.getByText( - "Partition 0 | Current offset 0 | End offset 0 | Lag 0" - ) - ).toBeVisible(); + expect(findTerm("Consumer offsets")).toBeVisible(); + expect(findDefinition("Fetch offsets to display data.")).toBeVisible(); expect( screen.queryByText("Service account password") ).not.toBeInTheDocument(); }); + + it("fetches Consumer offsets data when Fetch offset button is clicked", async () => { + const fetchButton = screen.getByRole("button", { + name: "Fetch the consumer offsets of the current subscription", + }); + + await userEvent.click(fetchButton); + + expect(mockGetConsumerOffsets).toHaveBeenCalledWith({ + consumerGroupId: + defaultPropsNonAiven.selectedSubscription.consumergroup, + env: defaultPropsNonAiven.selectedSubscription.environment, + topicName: defaultPropsNonAiven.selectedSubscription.topicname, + }); + + expect( + findDefinition("Partition 0: Current offset 0 | End offset 0 | Lag 0") + ).toBeVisible(); + }); }); describe("should render correct data in details modal (Aiven consumer, non owner user)", () => { @@ -295,7 +307,7 @@ describe("TopicSubscriptionsDetailsModal.tsx", () => { jest.resetAllMocks(); }); - it("does not fetch data for consumer offset", async () => { + it("does not fetch data for Consumer offsets on load", async () => { expect(mockGetConsumerOffsets).not.toHaveBeenCalled(); }); diff --git a/coral/src/app/features/topics/details/subscriptions/components/TopicSubscriptionsDetailsModal.tsx b/coral/src/app/features/topics/details/subscriptions/components/TopicSubscriptionsDetailsModal.tsx index 56dac92208..5f0b6a5bd2 100644 --- a/coral/src/app/features/topics/details/subscriptions/components/TopicSubscriptionsDetailsModal.tsx +++ b/coral/src/app/features/topics/details/subscriptions/components/TopicSubscriptionsDetailsModal.tsx @@ -7,17 +7,12 @@ import { StatusChip, Typography, } from "@aivenio/aquarium"; -import { useQuery } from "@tanstack/react-query"; +import { useQuery, useQueryClient } from "@tanstack/react-query"; import { useEffect, useState } from "react"; import { Modal } from "src/app/components/Modal"; -import { - getAivenServiceAccountDetails, - getConsumerOffsets, -} from "src/domain/acl/acl-api"; -import { - ConsumerOffsets, - ServiceAccountDetails, -} from "src/domain/acl/acl-types"; +import { ConsumerOffsetsValues } from "src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues"; +import { getAivenServiceAccountDetails } from "src/domain/acl/acl-api"; +import { ServiceAccountDetails } from "src/domain/acl/acl-types"; import { AclOverviewInfo } from "src/domain/topic/topic-types"; import { HTTPError } from "src/services/api"; import { parseErrorMsg } from "src/services/mutation-utils"; @@ -26,7 +21,6 @@ interface TopicSubscriptionsDetailsModalProps { closeDetailsModal: () => void; isAivenCluster: boolean; selectedSubscription: AclOverviewInfo; - offsetsData?: ConsumerOffsets; serviceAccountData?: ServiceAccountDetails; } @@ -47,27 +41,9 @@ const TopicSubscriptionsDetailsModal = ({ aclPatternType, } = selectedSubscription; + const queryClient = useQueryClient(); const [errors, setErrors] = useState([]); - const { - data: offsetsData, - error: offsetsError, - isFetched: offsetsDataFetched, - } = useQuery( - ["consumer-offsets", topicname, environment, consumergroup], - { - queryFn: () => { - return getConsumerOffsets({ - topicName: topicname, - env: environment, - consumerGroupId: consumergroup || "", - }); - }, - // Offsets data is only available for Consumer subscriptions in non-Aiven clusters - enabled: topictype === "Consumer" && !isAivenCluster, - } - ); - const { data: serviceAccountData, error: serviceAccountError, @@ -89,19 +65,14 @@ const TopicSubscriptionsDetailsModal = ({ ); useEffect(() => { - if (offsetsError !== null) { - setErrors((prev) => [...prev, parseErrorMsg(offsetsError)]); - } if (serviceAccountError !== null) { setErrors((prev) => [...prev, parseErrorMsg(serviceAccountError)]); } - }, [offsetsError, serviceAccountError]); + }, [serviceAccountError]); const serviceAccountDataLoaded = serviceAccountDataFetched && serviceAccountData !== undefined; - const offsetsDataLoaded = offsetsDataFetched && offsetsData !== undefined; - const serviceAccountOrPrincipalText = isAivenCluster ? "Service account" : "Principal"; @@ -111,9 +82,15 @@ const TopicSubscriptionsDetailsModal = ({ title={"Subscription details"} primaryAction={{ text: "Close", - onClick: closeDetailsModal, + onClick: () => { + closeDetailsModal(); + queryClient.removeQueries({ queryKey: ["getConsumerOffsets"] }); + }, + }} + close={() => { + closeDetailsModal(); + queryClient.removeQueries({ queryKey: ["getConsumerOffsets"] }); }} - close={closeDetailsModal} > {errors.length > 0 && ( @@ -216,20 +193,16 @@ const TopicSubscriptionsDetailsModal = ({ - Consumer offset + Consumer offsets - {offsetsDataLoaded ? ( - - {`Partition ${offsetsData[0].topicPartitionId} | - Current offset ${offsetsData[0].currentOffset} | - End offset ${offsetsData[0].endOffset} | - Lag ${offsetsData[0].lag}`} - - ) : ( -
- -
- )} + + setErrors((prev) => [...prev, error]) + } + topicName={topicname} + environment={environment} + consumerGroup={consumergroup} + /> ) : null} From 3dc63fa0b65d0798e4e32c60b677dc61e627666c Mon Sep 17 00:00:00 2001 From: Mathieu Anderson Date: Thu, 23 Nov 2023 17:02:01 +0100 Subject: [PATCH 2/2] fix(core/coral): Make OffsetDetails properties required, remove prettier check on openapi.yaml Signed-off-by: Mathieu Anderson --- .../clusterapi/models/consumergroup/OffsetDetails.java | 9 +++++---- coral/package.json | 3 --- .../subscriptions/components/ConsumerOffsetsValues.tsx | 10 +++++----- coral/types/api.d.ts | 8 ++++---- .../model/cluster/consumergroup/OffsetDetails.java | 9 +++++---- .../io/aiven/klaw/model/response/OffsetDetails.java | 9 +++++---- openapi.yaml | 3 ++- 7 files changed, 26 insertions(+), 25 deletions(-) diff --git a/cluster-api/src/main/java/io/aiven/klaw/clusterapi/models/consumergroup/OffsetDetails.java b/cluster-api/src/main/java/io/aiven/klaw/clusterapi/models/consumergroup/OffsetDetails.java index 2ffb4553c8..0827e855f5 100644 --- a/cluster-api/src/main/java/io/aiven/klaw/clusterapi/models/consumergroup/OffsetDetails.java +++ b/cluster-api/src/main/java/io/aiven/klaw/clusterapi/models/consumergroup/OffsetDetails.java @@ -1,14 +1,15 @@ package io.aiven.klaw.clusterapi.models.consumergroup; +import jakarta.validation.constraints.NotNull; import lombok.Data; @Data public class OffsetDetails { - private String topicPartitionId; + @NotNull private String topicPartitionId; - private String currentOffset; + @NotNull private String currentOffset; - private String endOffset; + @NotNull private String endOffset; - private String lag; + @NotNull private String lag; } diff --git a/coral/package.json b/coral/package.json index f9304d8156..f33e31f5a9 100644 --- a/coral/package.json +++ b/coral/package.json @@ -36,9 +36,6 @@ ], "**/*.{md, css}": [ "prettier --check" - ], - "../openapi.yaml": [ - "prettier --check" ] }, "dependencies": { diff --git a/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.tsx b/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.tsx index 02b1ae03d6..463154ecbe 100644 --- a/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.tsx +++ b/coral/src/app/features/topics/details/subscriptions/components/ConsumerOffsetsValues.tsx @@ -15,10 +15,10 @@ interface ConsumerOffsetsProps { } const parseOffsetsContent = ({ - topicPartitionId = "Unknown", - currentOffset = "Unknown", - endOffset = "Unknown", - lag = "Unknown", + topicPartitionId, + currentOffset, + endOffset, + lag, }: ConsumerOffsets) => { return `Partition ${topicPartitionId}: Current offset ${currentOffset} | End offset ${endOffset} | Lag ${lag}`; }; @@ -88,7 +88,7 @@ const ConsumerOffsetsValues = ({ offsetsData.map((data, index) => { return ( {parseOffsetsContent(data)} diff --git a/coral/types/api.d.ts b/coral/types/api.d.ts index 5f0435d30e..50e5ccc641 100644 --- a/coral/types/api.d.ts +++ b/coral/types/api.d.ts @@ -1540,10 +1540,10 @@ export type components = { teamMembersCount?: number; }; OffsetDetails: { - topicPartitionId?: string; - currentOffset?: string; - endOffset?: string; - lag?: string; + topicPartitionId: string; + currentOffset: string; + endOffset: string; + lag: string; }; KafkaConnectorRequestsResponseModel: { environment: string; diff --git a/core/src/main/java/io/aiven/klaw/model/cluster/consumergroup/OffsetDetails.java b/core/src/main/java/io/aiven/klaw/model/cluster/consumergroup/OffsetDetails.java index 2dc8facf39..26b3944060 100644 --- a/core/src/main/java/io/aiven/klaw/model/cluster/consumergroup/OffsetDetails.java +++ b/core/src/main/java/io/aiven/klaw/model/cluster/consumergroup/OffsetDetails.java @@ -1,14 +1,15 @@ package io.aiven.klaw.model.cluster.consumergroup; +import jakarta.validation.constraints.NotNull; import lombok.Data; @Data public class OffsetDetails { - private String topicPartitionId; + @NotNull private String topicPartitionId; - private String currentOffset; + @NotNull private String currentOffset; - private String endOffset; + @NotNull private String endOffset; - private String lag; + @NotNull private String lag; } diff --git a/core/src/main/java/io/aiven/klaw/model/response/OffsetDetails.java b/core/src/main/java/io/aiven/klaw/model/response/OffsetDetails.java index 67a8af9e77..957b1ddc69 100644 --- a/core/src/main/java/io/aiven/klaw/model/response/OffsetDetails.java +++ b/core/src/main/java/io/aiven/klaw/model/response/OffsetDetails.java @@ -1,14 +1,15 @@ package io.aiven.klaw.model.response; +import jakarta.validation.constraints.NotNull; import lombok.Data; @Data public class OffsetDetails { - private String topicPartitionId; + @NotNull private String topicPartitionId; - private String currentOffset; + @NotNull private String currentOffset; - private String endOffset; + @NotNull private String endOffset; - private String lag; + @NotNull private String lag; } diff --git a/openapi.yaml b/openapi.yaml index 8995e2bd3b..c3afc1309c 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -8777,7 +8777,8 @@ "lag" : { "type" : "string" } - } + }, + "required" : [ "currentOffset", "endOffset", "lag", "topicPartitionId" ] }, "KafkaConnectorRequestsResponseModel" : { "properties" : {