-
Notifications
You must be signed in to change notification settings - Fork 367
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
Sj/feat/kafka connect impl #910
Sj/feat/kafka connect impl #910
Conversation
41043fd
to
d5d37d6
Compare
// GetStatusCodeFromAPIError tries to parse given error as kafa connect | ||
// ApiError and returns the status code, if parsing is not possible it returns | ||
// a fallback error code | ||
func GetStatusCodeFromAPIError(err error, fallback int) int { |
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.
Forward the actual error code of the response when possible
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.
Maybe we should check if the status code is within our expected range (200, 202, 400-503) or the likes, to ensure we are not getting non-sense back from Kafka connect. If it's not within the range we'll also return the fallback. The API reference says they will do so, but may introduce 3xx as well at some point. Just to ensure we are not bringing ourselves in trouble whenever they change something there (e.g. in schema registry they use custom error codes like 40401
).
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.
I only account for 400 and 500 status code since those are the errors that should be present
d5d37d6
to
a8e6567
Compare
@sago2k8 If we use an array in the response, we can't introduce pagination without a breaking API change. We try to provide paginated responses where reasonable. Debatable whether this is the case here (or on other responses). |
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.
Good job, left a few minor comments 👍
enum ExpandOption { | ||
// buf:lint:ignore ENUM_ZERO_VALUE_SUFFIX | ||
UNSPECIFIED = 0; | ||
// buf:lint:ignore ENUM_VALUE_UPPER_SNAKE_CASE | ||
status = 1; | ||
// buf:lint:ignore ENUM_VALUE_UPPER_SNAKE_CASE | ||
info = 2; | ||
} |
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.
To be discussed for all enums on the kafka connect API. How much do we want to remain Kafka connect API compatible versus consistency & follow best practices for the Redpanda API.
Santiago opted for best possibly compatibility with the Kafka Connect API when it comes to enums as far as I can tell.
@birdayz any opinion?
message ListConnectorsRequest { | ||
repeated ExpandOption expand = 1 [(buf.validate.field).repeated.max_items = 2]; | ||
string cluster_name = 2 [ | ||
(google.api.field_behavior) = REQUIRED, |
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.
Is this field behavior annotation required if we already set the validate annotation? As far as I remember it the buf validate annotations are already considered for client code generation, but I may be wrong. Same applies to the remaining fields where both is set
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.
This is useful for the generated documentation, this field will be marked as required in the Swagger generated documentation.
rpc ListConnectors(ListConnectorsRequest) returns (ListConnectorsResponse) { | ||
option (google.api.http) = { | ||
get: "/v1alpha1/{cluster_name}/connectors" | ||
response_body: "connectors" |
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.
Plain array response may prevent us from adding pagination. Let's discuss whether we need it or not.
abbd8a2
to
5767991
Compare
3d3df09
to
2e4e5bb
Compare
2e4e5bb
to
5382071
Compare
} | ||
|
||
message ListConnectorsRequest { | ||
repeated string expand = 1 [ |
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.
What do you think about removing the expand parameter and always setting both info and status to true? Personally I don't find this very idiomatic, but at least we have a stable response schema now, so that I'm fine with keeping it as well.
To me it feels like they just added these as optional parameters at a later point to remain 100% backwards compatible (no additional fields added in newer versions).
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.
That makes sense removing it
|
||
message CreateConnectorResponse { | ||
string name = 1; | ||
// cod fig is the connector cron configuration map, every key is a string |
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.
I don't understand this comment
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.
fixed
optional ConnectorSpec info = 2; | ||
optional ConnectorStatus status = 3; |
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.
If we remove the optional expand parameter we can remove the optional.
// Connect equivalent REST endpoint | ||
rpc GetConnector(GetConnectorRequest) returns (GetConnectorResponse) { | ||
option (google.api.http) = { | ||
get: "/v1alpha1/kafka-connect/clusters/{cluster_name}/connectors/{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.
Let's present the URL path in the sync. I can imagine it may be controversial, because:
- It contains
kafka-connect
. We maybe want to usemanaged-connectors
for Cloud, but not for community. - We always have a single kafka connect cluster in cloud
taskfiles/proto.yaml
Outdated
@@ -84,6 +84,8 @@ tasks: | |||
vars: | |||
VERSION: 0.2.2 |
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.
Change to 0.3.1
taskfiles/proto.yaml
Outdated
@@ -84,6 +84,8 @@ tasks: | |||
vars: | |||
VERSION: 0.2.2 | |||
cmds: | |||
- 'go install go.vallahaye.net/connect-gateway/cmd/protoc-gen-connect-gateway@d941a13c0fbf10931098de2fd084d3ee8d0bd1e4' | |||
- | | |||
GOBIN={{ .BUILD_ROOT }}/bin/go go install go.vallahaye.net/connect-gateway/cmd/protoc-gen-connect-gateway@d941a13c0fbf10931098de2fd084d3ee8d0bd1e4 |
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.
Reference version from var again
// this includes holistic unified connector status that takes into account not | ||
// just the connector instance state, but also state of all the tasks within the | ||
// connector | ||
enum HolisticState { |
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.
Maybe ConnectorHolisticState
or just ConnectorState
. The name HolisticState for the entire API package may be too generic.
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.
ConnectorState may be clashing with the state that Kafka connect already reports for a Connector though. This state is kind of the reason why we introduced our "own state" that considers the task states as well as the connector states.
dee540f
to
c3c82d6
Compare
Implement messages for list connectors and extend tooling for the proto generate task Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
c3c82d6
to
1910290
Compare
Extend Kafka Connect service abstraction so we expose more details and the correct http error code Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
Implement grcp connect for the kafka-connect service, add mapper for transforming the kafka connect responses into a valid grpc connect responses, errors etc Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
1910290
to
2c2c984
Compare
Add a simple integration test, to validate an error case Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
- Update testcontainers to the latest version. - use methods of testcontainers/modules/redpanda for retrieving schema registry - Update action since the issues linked for the override have been resolved Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
2c2c984
to
7797e67
Compare
Thanks for the review @weeco I addressed your comments :) |
Initial impl of the kafka connect api for the dataplane api.
ref:
key points:
Add support for custom http response status code. I.E
202
,204
Forward the actual status code from the kafka-connect response
Add preliminar protos for some kafka connect methods/endpoints
Kafka connect endpoints implemented:
onlyFailed
andincludeTasks
,The schema of the request and responses is more or less similar to Confluent but Errors and List request, which are heavily inspired by our ControlPlane API and GRPC-connect/REST grpc gateway. style, this was discussed and we consider UX over compatibility.
There are some changes done around integration tests, dependency updates and refactor some test consuming test-containers.