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.

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

pkg/ui: Swap in new Snapshot debug component #94701

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

benbardin
Copy link
Collaborator

@benbardin benbardin commented Jan 4, 2023

The PR focuses on the routes. I'll delete deprecated code in a follow-up diff.

Part of: #83679

Release note: None

@benbardin benbardin requested review from andreimatei, dhartunian and a team January 4, 2023 16:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @benbardin)


pkg/ui/workspaces/cluster-ui/src/tracez/snapshot/snapshotPage.tsx line 326 at r1 (raw file):

    ],
    [nodeID, snapshotID, spanID, spanOperation],
  );

is useMemo necessary here? the operation doesn't look expensive.


pkg/ui/workspaces/cluster-ui/src/tracez/snapshot/snapshotPage.tsx line 341 at r1 (raw file):

    // eslint-disable-next-line react-hooks/exhaustive-deps
    [nodeID, snapshotID, spanID, spanOperation],
  );

same comment as above.

Copy link
Collaborator Author

@benbardin benbardin left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @dhartunian)


pkg/ui/workspaces/cluster-ui/src/tracez/snapshot/snapshotPage.tsx line 326 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

is useMemo necessary here? the operation doesn't look expensive.

Done.


pkg/ui/workspaces/cluster-ui/src/tracez/snapshot/snapshotPage.tsx line 341 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

same comment as above.

Done.

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)

@benbardin
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 11, 2023

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

craig bot pushed a commit that referenced this pull request Jan 11, 2023
94701: pkg/ui: Swap in new Snapshot debug component r=benbardin a=benbardin

The PR focuses on the routes. I'll delete deprecated code in a follow-up diff.

Part of: #83679

Release note: None

Co-authored-by: Ben Bardin <[email protected]>
@rail
Copy link
Member

rail commented Jan 11, 2023

bors r-
We have temporarily disabled bors for merges. Please merge the PR manually when all required checks pass.

@craig
Copy link
Contributor

craig bot commented Jan 11, 2023

Canceled.

@benbardin benbardin merged commit 3d02033 into cockroachdb:master Jan 11, 2023
@benbardin benbardin deleted the newtrace branch January 11, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants