Skip to content
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.

Sign up for GitHub

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

Jump to bottom

deprisma pt.1: API and UI cleanup #1880

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

deprisma pt.1: API and UI cleanup #1880

wants to merge 6 commits into from

Conversation

heavycrystal
Copy link
Contributor

had started with just Prisma cleanup but turned into a general cleanup so splitting off to prevent code churn

  1. removed drop mirror endpoint, switched to using flow state change endpoint with status TERMINATED
  2. migration to make some row counts BIGINT
  3. eliminated some Prisma calls, moved the rest back to route.ts files under /api
  4. Resync for CDC mirrors is now an endpoint, prevent having to read the proto and then trigger multiple endpoints
  5. remove some DTO types and replace with protos
  6. protos remove fields

serprex
serprex reviewed Jun 26, 2024
View reviewed changes
ui/app/mirrors/[mirrorId]/qrepStatusTable.tsx Outdated
@@ -98,7 +91,7 @@ export default function QRepStatusTable({ partitions }: QRepStatusTableProps) {
currentPartitions.sort((a, b) => {
const aValue = a[sortField];
const bValue = b[sortField];
if (aValue === null || bValue === null) {
if (aValue === undefined || bValue === undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this behavior seems kinda weird, usually you want nulls to go to end or start, not randomly plinko through sort

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to also be fixed in other places that sort on multiple types

serprex
serprex reviewed Jun 27, 2024
View reviewed changes
protos/flow.proto
@@ -302,7 +302,7 @@ message QRepParitionResult {
}

message DropFlowInput {
string flow_name = 1;
string flow_job_name = 1;
Copy link
Member

@serprex serprex Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renaming fields breaks things, but can probably get by on this proto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proto wasn't used before this PR for some reason

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iskakaushik iskakaushik iskakaushik left review comments

@serprex serprex serprex approved these changes

@Amogh-Bharadwaj Amogh-Bharadwaj Awaiting requested review from Amogh-Bharadwaj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@heavycrystal @serprex @iskakaushik