Skip to content

Conversation

@C0urante
Copy link
Contributor

@C0urante C0urante commented Feb 17, 2022

@C0urante
Copy link
Contributor Author

Converting to draft until upstream PRs are reviewed.

@C0urante
Copy link
Contributor Author

Given that all merge conflicts have been resolved and #11775 has already been approved, marking this as ready for review.

@C0urante C0urante marked this pull request as ready for review May 10, 2022 00:08
@C0urante C0urante force-pushed the kafka-10000-preflight-validation branch from 7c20275 to 923af52 Compare May 17, 2022 12:22
@C0urante
Copy link
Contributor Author

C0urante commented May 21, 2022

@showuon the 3.3.0 feature freeze is a little over a month away and there are still seven open pull requests for this feature. Do you have time to take a look?

@C0urante
Copy link
Contributor Author

@showuon If you do not have time to take a look, is there anyone you can recommend in your place?

@C0urante
Copy link
Contributor Author

C0urante commented Jun 1, 2022

@showuon hello? Is anyone actually maintaining Kafka Connect anymore or should we all just stop contributing to it?

Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

A few nits, but LGTM

Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to assume validatedName != null at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; every framework-level connector property should have a ConfigValue entry in the map here.

Comment on lines 974 to 973
Copy link
Member

Choose a reason for hiding this comment

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

Factor out this else if/else logic into a method, since it's the same between the connectorUsesConsumer and connectorUsesAdmin methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done.

Copy link
Member

Choose a reason for hiding this comment

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

Could we assert on error message equality (or contains if there can be more than one)? As well was making the tests clearer it would make it clearer that we're actually hitting the expected validation condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, done.

@C0urante C0urante force-pushed the kafka-10000-preflight-validation branch from 923af52 to 6f420b1 Compare June 1, 2022 21:52
@C0urante
Copy link
Contributor Author

C0urante commented Jun 1, 2022

Thanks Tom, addressed the nits.

Copy link
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks @C0urante !

@mimaison mimaison merged commit a110f1f into apache:trunk Jun 2, 2022
@C0urante C0urante deleted the kafka-10000-preflight-validation branch June 2, 2022 12:26
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.

3 participants