-
Notifications
You must be signed in to change notification settings - Fork 389
upcoming: [UIE-9378] - DBaaS - Display connection pool section and table in Networking tab #13195
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
base: develop
Are you sure you want to change the base?
upcoming: [UIE-9378] - DBaaS - Display connection pool section and table in Networking tab #13195
Conversation
| ._ctx.pools, | ||
| ...databaseQueries | ||
| .database('postgresql', databaseId) | ||
| ._ctx.connectionPools._ctx.paginated(params), |
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.
The connection pools query needed to accept pagination parameters, so I've updated this query and made a change in the keys.ts file. I also updated function name to include query for consistency with other similar functions in the file.
| flexDirection: 'column', | ||
| }, | ||
| }, | ||
| })); |
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.
These were common styles that I ended up reusing for the ActionsMenuWrapper used in the landing page table and for the new table for connection pools and for positioning settings items.
I've moved these shared styles to this new shared.styles.ts file to make it easier to reuse them. These styles can be seen in the DatabaseManageNetworking and the new DatabaseConnectionPools and DatabaseRow components. You'll see them pointing to this file now.
There may be other styles we can move here, so I plan to move them here as I go through other changes and come across them.
| <DatabaseManageNetworking database={database} /> | ||
| {flags.databasePgBouncer && database.engine === 'postgresql' && ( | ||
| <DatabaseConnectionPools database={database} /> | ||
| )} |
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.
Added these checks at the networking tab to conditionally render the new section based on the feature flag and database engine. It will only appear for postgresql database clusters.
|
|
||
| const flags = useFlags(); | ||
| const { classes } = useStyles(); | ||
| const { classes } = makeSettingsItemStyles(); |
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.
Moved these styles above to a shared file. See my other comment
| color: theme.tokens.alias.Content.Icon.Primary.Hover, | ||
| }, | ||
| })); | ||
| })(({ theme }) => getActionMenuWrapperStyles(theme)); |
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.
Moved these styles above to a shared file. See my other comment
| ), | ||
| setMethod('GET'), | ||
| setParams(params), | ||
| ); |
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.
Updated to accept params as we need to be able to pass pageSize and pageNumber to the request.
hana-akamai
left a comment
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.
...manager/src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPools.tsx
Outdated
Show resolved
Hide resolved
...manager/src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPools.tsx
Outdated
Show resolved
Hide resolved
...manager/src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPools.tsx
Outdated
Show resolved
Hide resolved
| <div style={{ overflowX: 'auto', width: '100%' }}> | ||
| <Table | ||
| aria-label={'List of Connection pools'} | ||
| style={ |
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.
Can we move this style into a styled component since it's taking up a few lines?
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.
Do you mean a styled table component?
...manager/src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPools.tsx
Outdated
Show resolved
Hide resolved
| page={pagination.page} | ||
| pageSize={pagination.pageSize} | ||
| pageSizes={PAGE_SIZES} | ||
| style={{ |
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.
Same here, can we move styles to a styled component?
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.
As discussed, we'll look into addressing the additional styled components separately.
1a7819d to
fa11cc7
Compare
| .database('postgresql', databaseId) | ||
| ._ctx.connectionPools._ctx.pool(poolName), | ||
| enabled, | ||
| refetchInterval: 20000, |
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.
Is this refetchInterval intentional?
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.
ah right! I copied that logic from the calls we make for the table. I wasn't sure if we needed to refetch this. I can remove it for now and verify with their team if we need to refetch this data. I they do want to refetch it, I can verify how often we should and add it back later. Would that work?
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've removed it for now as it looks like it's applied to the wrong query. This was meant for the table data query. I can ask the backend team about refetching to see if we need it and how often it should be.
bnussman-akamai
left a comment
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.
Looks good! Just had one question about loading/error states in this context
| if (connectionPoolsLoading) { | ||
| return <CircleProgress />; | ||
| } |
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.
Is this the desired loading and error state?
We could inline the loading state and error state into the table itself using https://design.linode.com/?path=/docs/components-table-tablerowloading--documentation
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.
@bnussman-akamai So the DatabaseConnectionPool component is currently using the cds table web component that was integrated in a previous pull request (#12989) for the database landing page. I opted to use the same setup from the DatabaseLanding for this component for consistency.
After checking the code for the cds-web component table, that table doesn't have an inline error state/loading behavior like we see in the Linode Table component linked above. So we can't implement this behavior for the connection pool table. So for now, I'm planning to follow the same state pattern we see on the landing page.
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.
Ah right. My fault. I forgot DBaaS uses the CDS Web component table. If there isn't an equivalent I don't have any issues with the current approach
…ble in Networking tab
…r and applying paginator min, results and page sizes
a631c31 to
8fee513
Compare
Cloud Manager UI test results🎉 865 passing tests on test run #6 ↗︎
|



Description 📝
This pull request is an update to DBaaS to display the Connection Pools section and table in the Networking tab in database details.
Changes 🔄
List any change(s) relevant to the reviewer.
Scope 🚢
Upon production release, changes in this PR will be visible to:
Target release date 🗓️
TBD
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
databasePgBouncerfeature flag enabled. This will appear asDatabase PgBouncerin the dev tools.databasevariable and add the following lines:Verification steps
(How to verify changes)
Verifying Display Behavior for section and table
postgresqldatabases, the newManage PgBouncer Connection Poolssection is now displayedVerify Feature Flag Rendering Checks
databasePgBouncerfeature flagManage PgBouncer Connection Poolssection is no longer displayed in the Networking tabVerify PostgreSQL Engine Check
databasePgBouncerfeature flagmysqlinstead.Manage PgBouncer Connection Poolsis no longer displayed even though the feature flag is switched on.Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅