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

feat: Implemented additional REST API methods #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

richardcase
Copy link

Added support for the following:

  • Getting the status of a specific task for a connector
  • Restarting a specific task for a connector
  • Getting a list of connector plugins

Signed-off-by: Richard Case 198425+richardcase@users.noreply.github.com

Added support for the following:
- Getting the status of a specific task for a connector
- Restarting a specific task for a connector
- Getting a list of connector plugins

Signed-off-by: Richard Case <198425+richardcase@users.noreply.github.com>
Copy link
Member

@ches ches left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @richardcase!

The CI build got broken by golint changing its import path… sorry about that, I've fixed it on master if you wouldn't mind rebasing this PR branch (nothing will conflict).

Seeing that naming becomes a bit cumbersome for a nested resource (like GetConnectorTaskStatus(name string, taskID int)), it leads me to consider removing the "Connector" qualifier for Task methods before a stable 1.0 version… GetTaskStatus(connector string, taskID int) is probably nicer, and it seems unlikely that "task" will become an ambiguous entity in the Kafka Connect API at this point. Thoughts or objections?

That would need refactoring of existing code like GetConnectorTasks which I'd rather do separately, so it's not a request for you to rename the new methods now (but if you'd like to so that you can use this from master as soon as it's merged and not have it change later, that's cool with me). Otherwise I'll try to get that done quickly and tag a v0.10.0.

// ListPlugins retrieves a list of the installed plugins.
//
// See: https://docs.confluent.io/current/connect/references/restapi.html#get--connector-plugins-
func (c *Client) ListPlugins() ([]*Plugin, *http.Response, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to return []Plugin here rather than pointers, consistent with GetConnectorTasks. Since these are small structs holding only simple types and the collection will be small, sticking to the stack should be fine.

Also, I think I'd go with GetPlugins() as the method name—trying to recall why there is the seeming inconsistency of e.g. ListConnectors() vs. GetConnectorTasks, I believe it was because the GET /connectors API returns an array of scalar names, it's not a very "resourceful" REST endpoint and the method thus doesn't pair so well with GetConnector(name string). For entity-like resource collections I prefer the GetResources naming convention.

// GetConnectorTaskStatus gets the status of task for a connector.
//
// See: https://docs.confluent.io/current/connect/references/restapi.html#get--connectors-(string-name)-tasks-(int-taskid)-status
func (c *Client) GetConnectorTaskStatus(name string, taskID int) (*TaskState, *http.Response, error) {
Copy link
Member

Choose a reason for hiding this comment

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

On second thought, could these methods take a TaskID as single argument, instead of the weaker constituent basic types?

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.

None yet

2 participants