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

Implement suspend and resume endpoints of supervisor API #7

Merged
merged 13 commits into from
Dec 12, 2023

Conversation

tomasz-h2o
Copy link
Collaborator

@tomasz-h2o tomasz-h2o commented Oct 5, 2023

This PR is on hold as a work of extended deduplication.

Addresses https://github.com/h2oai/telemetry/issues/167

@vzayts vzayts added the area/core Core code related issue label Dec 8, 2023
@vzayts vzayts marked this pull request as ready for review December 8, 2023 20:36
@vzayts
Copy link

vzayts commented Dec 8, 2023

Opening this one for review.

supervisor.go Outdated
Comment on lines 125 to 128
var result OutputIngestionSpec
if err != nil {
return result, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style/Optional:

  1. Usually, error handling should come right after the call that returns error.
  2. I'd say it's usually safer to return explicit zero value in err branches.

(applies to Resume as well)

Copy link

Choose a reason for hiding this comment

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

fixed

Comment on lines +153 to 155
type QueryGranularity struct {
Type string `json:"type,omitempty"`
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type QueryGranularity struct {
Type string `json:"type,omitempty"`
}
type QueryGranularity struct {
Type string `json:"type,omitempty"`
}

And add a comment please.

supervisor_types.go Outdated Show resolved Hide resolved
supervisor_types.go Show resolved Hide resolved
supervisor_types.go Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@tomasz-h2o tomasz-h2o left a comment

Choose a reason for hiding this comment

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

Please address @zoido's comments WRT to unmarshalling.

@vzayts vzayts requested a review from zoido December 11, 2023 20:58
supervisor.go Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@tomasz-h2o tomasz-h2o left a comment

Choose a reason for hiding this comment

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

LGTM (I can't approve as PR creator)

var result SupervisorStatus
r, err := s.client.NewRequest("GET", applySupervisorId(supervisorStatusEndpoint, supervisorId), nil)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nit: please consider using http.MethodGet

Co-authored-by: Luboš Pokorný <[email protected]>
@vzayts vzayts merged commit af2aa0d into master Dec 12, 2023
3 checks passed
@vzayts vzayts deleted the tomasz/implement-suspend-resume branch December 12, 2023 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core Core code related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants