-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: add zookeeper current metrics #241
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Hey @duyet - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -50,7 +50,7 @@ export async function RelatedCharts({ | |||
|
|||
return ( | |||
<div className={cn('mb-5 grid gap-5', gridCols, className)}> | |||
{charts.map(([_name, Chart, props], i) => { | |||
{charts.map(([name, Chart, props], i) => { |
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.
suggestion: Consider renaming 'name' to 'chartName' for clarity.
Renaming 'name' to 'chartName' can improve readability and avoid potential conflicts with other variables named 'name' in the broader scope.
{charts.map(([name, Chart, props], i) => { | |
{charts.map(([chartName, Chart, props], i) => { |
console.debug( | ||
`${name} add col-span-2 due to next chart being a break` | ||
) |
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.
issue (performance): Remove or conditionally compile debug statements.
Leaving debug statements in production code can lead to performance issues and cluttered logs. Consider removing them or using a conditional compilation approach.
@@ -22,6 +22,7 @@ export async function ChartReplicationSummaryTable({ | |||
COUNT() as total | |||
FROM system.replication_queue | |||
GROUP BY 1, 2 | |||
ORDER BY total DESC |
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.
suggestion (performance): Consider adding an index on the 'total' column.
Ordering by 'total' might benefit from an index on the 'total' column to improve query performance, especially with large datasets.
ORDER BY total DESC | |
CREATE INDEX idx_total ON system.replication_queue (total); | |
ORDER BY total DESC |
{Object.values(row).map((value, i) => { | ||
return <TableCell key={i}>{value || ''} </TableCell> |
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.
issue: Avoid using index as key in list rendering.
Using the index as a key can lead to issues with component state and performance. Consider using a unique identifier from the data instead.
}[] | ||
>({ query }) | ||
|
||
const uptime = rows[0] || { uptime: 'N/A' } |
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.
issue: Handle case where rows array is empty.
Ensure that the code handles the case where the rows array is empty to avoid potential runtime errors.
No description provided.