Skip to content

KAFKA-12770: introduce checkstyleVersion build option (for overriding CheckStyle project-defined dependency version)#10698

Closed
dejan2609 wants to merge 1 commit intoapache:trunkfrom
dejan2609:KAFKA-12770
Closed

KAFKA-12770: introduce checkstyleVersion build option (for overriding CheckStyle project-defined dependency version)#10698
dejan2609 wants to merge 1 commit intoapache:trunkfrom
dejan2609:KAFKA-12770

Conversation

@dejan2609
Copy link
Contributor

@dejan2609 dejan2609 commented May 14, 2021 •
edited
Loading

@ijuma please review this.

Rationale: Requested by CheckStyle team (@romani); they needs this in order to add Apache Kafka project into their regression suit here https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-to-test-on.properties

JIRA ticket: https://issues.apache.org/jira/browse/KAFKA-12770

Related PR/comment: #10656 (comment)

@romani how to use this (hopefully this will be merged into trunk at some point):

  • this command uses project-defined CheckStyle version: ./gradlew checkstyleMain checkstyleTest
  • while this one overrides it: ./gradlew checkstyleMain checkstyleTest -PcheckstyleVersion=8.41.1

@dejan2609 dejan2609 force-pushed the KAFKA-12770 branch 2 times, most recently from 9a624b9 to a80830e Compare May 15, 2021 20:53
@dejan2609
Copy link
Contributor Author

dejan2609 commented May 15, 2021

Note: rebased onto trunk (in order to pick up Gradle 6 -->> 7 version upgrade).

@dejan2609 dejan2609 mentioned this pull request May 16, 2021
Closed
@dejan2609
Copy link
Contributor Author

dejan2609 commented May 16, 2021

One flaky test failed, so I will retest this:

Build / JDK 11 and Scala 2.13 / testCreateClusterAndCreateListDeleteTopic() – kafka.server.RaftClusterTest19s
Error
java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.UnknownServerException: The server experienced an unexpected error when processing the request.

@dejan2609
Copy link
Contributor Author

dejan2609 commented May 16, 2021

retest this please

@romani
Copy link

romani commented May 16, 2021

as soon it is merged, we can add kafka to regression suite to make sure we will not leak regression on kafka code any more.

@dejan2609
Copy link
Contributor Author

dejan2609 commented May 16, 2021

Roger that @romani ! I hope that @ijuma will review this PR when he finds some time.

Note: previous round of tests returned one error (flaky test, it seems) so I tried to trigger a new series of tests executions via retest this please comment but nothing happened...

@dejan2609 dejan2609 mentioned this pull request May 16, 2021
Closed
showuon
showuon reviewed May 19, 2021
View reviewed changes
Copy link
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

@dejan2609 , thanks for the PR. Overall LGTM. Left a minor comment. Thank you.

@dejan2609 dejan2609 force-pushed the KAFKA-12770 branch 2 times, most recently from 1bb283e to 0280296 Compare May 19, 2021 16:37
@dejan2609
Copy link
Contributor Author

dejan2609 commented May 19, 2021

@showuon thanx for a review, feel free to leave a comment after I polished a little bit (or open another review, any critique is welcomed !).

@ijuma I know you have a lot on your plate, but please take a moment for this PR: it is a prerequisite for CheckStyle team to include Kafka into their regression suite.

@dejan2609
Copy link
Contributor Author

dejan2609 commented May 21, 2021

@ijuma all tests are in green. Can this be reviewed (and hopefully merged) ?
Sorry for pushing, but CheckStyle team is waiting for this.

@showuon
Copy link
Member

showuon commented May 22, 2021

@dejan2609 , sorry, I didn't make it clear. What I meant is here, there's a checkstyle section describing what checkstyle is and how to run it. So I think we can add a note there to mention the checkstyleVersion argument can be passed to override default checkstyle version to do some experiments and regression testing. What do you think?

@dejan2609
KAFKA-12770: introduce checkstyleVersion build option (for overridi…
0079e1f 
…ng CheckStyle project-defined dependency version)

rationale/notes:
* useful for experimenting and regression testing
* requested by a CheckStyle team so they can add Kafka into their regression suite
@dejan2609
Copy link
Contributor Author

dejan2609 commented May 22, 2021

Makes sense @showuon ! I totally understand your point: writing concise comments is a vital thing.

I made some changes according to your suggestion, feel free to check (your input here is highly appreciated).

showuon
showuon approved these changes May 23, 2021
View reviewed changes
Copy link
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@dejan2609
Copy link
Contributor Author

dejan2609 commented May 24, 2021 •
edited
Loading

If I may ask you @showuon: do we need to ping someone else for another review (or for merge into trunk) ?

Edit (just to answer to my self): there are two types of approvals and hence this PR does need an additional review(s)... in any case: thanx @showuon !

@dejan2609
Copy link
Contributor Author

dejan2609 commented May 24, 2021

CheckStyle team (@romani) needs this in order to add Kafka project into their regression suit here:
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-to-test-on.properties

@showuon was so kind to provide his assistance, but we now need approval by someone who has a write access (@mumrah, @guozhangwang, @hachikuji, @ijuma, @junrao, @cmccabe or someone else).

Note: changes are small and simple (and risk doesn't exist).

@dejan2609
Copy link
Contributor Author

dejan2609 commented Jun 2, 2021 •
edited
Loading

Just tagging @ijuma here again (to bring back this PR up to the surface).

@dejan2609 dejan2609 mentioned this pull request Jun 2, 2021
Closed
3 tasks
@dongjinleekr
Copy link
Contributor

dongjinleekr commented Jun 3, 2021

Cool. Working Like a charm.

./gradlew checkstyleMain checkstyleTest:
1
./gradlew checkstyleMain checkstyleTest -PcheckstyleVersion=8.41.1:
2

@dejan2609
Copy link
Contributor Author

dejan2609 commented Jun 12, 2021

@ijuma update: I already gather two approvals (made by fellow-contributors) for this PR, but I still need some maintainer to have his say.

@dejan2609 dejan2609 mentioned this pull request Jun 16, 2021
Merged
@dejan2609 dejan2609 mentioned this pull request Jul 3, 2021
Merged
@dejan2609
Copy link
Contributor Author

dejan2609 commented Jul 3, 2021

Closed in favor of #10967; git commit is identical, but it seems that this PR (with so many comments) is just out of sight.

@dejan2609 dejan2609 closed this Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@showuon showuon showuon approved these changes

+1 more reviewer

@dongjinleekr dongjinleekr dongjinleekr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dejan2609 @romani @showuon @dongjinleekr