-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Optimize desktop backend reads #52070
base: master
Are you sure you want to change the base?
Conversation
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 reasonable to me, should we add any test coverage or benchmarks for GetWindowsDesktops
?
c61049e
to
5a8ae17
Compare
5a8ae17
to
0dc4e59
Compare
// If both HostID and Name are set in the filter only one desktop should be expected | ||
if req.HostID != "" && req.Name != "" && len(desktops) == 0 { | ||
return nil, trace.NotFound("windows desktop \"%s/%s\" doesn't exist", req.HostID, req.Name) | ||
} | ||
|
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.
Have you checked around to see if we're depending on this for something?
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.
Yeah, I didn't spot anything.
ListWindowsDesktops
was added to support the unified resources view. It wasn't around when all the original desktop code was added, so I'm not surprised that nothing in the desktop code depends on it.
|
||
return desktops, nil | ||
} | ||
|
||
// GetWindowsDesktops returns all Windows desktops matching filter. | ||
func (s *WindowsDesktopService) GetWindowsDesktops(ctx context.Context, filter types.WindowsDesktopFilter) ([]types.WindowsDesktop, error) { |
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 use the same implementation as ListWindowsDesktops
, or are there differences?
We could keep the check on req.Limit
in ListWindowsDesktops
and delegate it and GetWindowsDesktops
to an internal function that doesn't check, so we can give it an infinite limit for GetWindowsDesktops
.
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 only difference right now is that when searching for a single desktop with name and host ID, GetWindowsDesktops
returns a not found error and ListWindowsDesktops
returns an empty result (but no error).
Updates #52062