You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/05/09 11:00:47 UTC
[GitHub] [kafka] dejan2609 opened a new pull request #10656: MINOR: code quality check tools (checkstyle and spotbugs): version upgrades
dejan2609 opened a new pull request #10656:
URL: https://github.com/apache/kafka/pull/10656
details:
* checkstyle: 8.20 -->> 8.36.2 https://checkstyle.org/releasenotes.html#Release_8.36.2
* spotbugs : 4.2.2 -->> 4.2.3 https://github.com/spotbugs/spotbugs/blob/4.2.3/CHANGELOG.md
Rationale:
* Checkstyle 8.36.2 is geared towards recent Java versions (see 8.36 release notes: https://checkstyle.org/releasenotes.html#Release_8.36)
* Checkstyle version 8.42 should be skiped (lots of false positives, see here: https://github.com/checkstyle/checkstyle/issues/9957)
* more recent Checkstyle versions (i.e. 8.37 and above) are imposing more strict indentation rules and hence we can opt to:
* relax Checkstyle indentation rules **_OR_**
* comply with these new rules and change affected classes (note: total of 50 violations in 18 classes is recorded when compiled with Checkstyle 8.41.1)
@ijuma Please review this.
Also, let me know what do you prefer: to merge this commit or wait for a full upgrade to a latest checkstyle version ? And in case that you chose to wait for an ugrade to a latest checkstyle version: which strategy do you prefer (i.e. are we going to relax checkstyle rules or comply with them) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] romani edited a comment on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
romani edited a comment on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835809154
@dejan2609, can you share she'll command that checkstyle team can execute to run only checkstyle validation, we will make sure no false positives happen.
Please share link to checkstyle config.
If you can enforce, no checkstyle violation in CI, and make it possible to change checkstyle version to any version from CLI arguments, checkstyle team can add Kafka project to regression suite and you will never have false positives from CheckStyle any more, as we will validate Kafka execution on each commit in checkstyle.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 edited a comment on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 edited a comment on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835815883
> @dejan2609, can you share she'll command that checkstyle team can execute to run only checkstyle validation, we will make sure no false positives happen.
> Please share link to checkstyle config.
Sure @romani ! I will provide you complete POC/manual on your repository / related ticket asap.
> If you can enforce, no checkstyle violation in CI, and make it possible to change checkstyle version to any version from CLI arguments, checkstyle team can add Kafka project to regression suite and you will never have false positives from CheckStyle any more, as we will validate Kafka execution on each commit in checkstyle.
I am only a (modest) apache-kafka contributor, but I do know that @ijuma can answer you about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 edited a comment on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 edited a comment on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835786890
Alas, spotbugs checks fail... https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-10656/1/pipeline
Let me check again on my end... converting to a draft.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 edited a comment on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 edited a comment on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835792416
My bad... I was cutting corners while creating this PR, so I just didn't check spotbugs related gradle tasks on my end... I was counting on a fact that this is just patch, but it turns our that spotbugs version 4.2.3 is not stable.
So, it is just a checkstyle upgrade, after all.
@ijuma feel free to review now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 edited a comment on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 edited a comment on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835815883
> @dejan2609, can you share she'll command that checkstyle team can execute to run only checkstyle validation, we will make sure no false positives happen.
> Please share link to checkstyle config.
Sure @romani ! I will provide you complete POC/manual on your repository / related ticket asap.
> If you can enforce, no checkstyle violation in CI, and make it possible to change checkstyle version to any version from CLI arguments, checkstyle team can add Kafka project to regression suite and you will never have false positives from CheckStyle any more, as we will validate Kafka execution on each commit in checkstyle.
Kafka is using Jenkins: https://github.com/apache/kafka/blob/2.8.0/Jenkinsfile#L28; it should not be that hard to implement your requirements (I did managed to pull out similar things on Jenkins) but I really can't speak for Apache Kafka project because I am only external contributor.
Kafka project maintainers should be involved here, I guess. FYI @mumrah and @ijuma
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] romani commented on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
romani commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835809154
@dejan2609, can you share she'll command that checkstyle team can execute to run only checkstyle validation, we will make sure no false positives happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 commented on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835815883
> @dejan2609, can you share she'll command that checkstyle team can execute to run only checkstyle validation, we will make sure no false positives happen.
> Please share link to checkstyle config.
Sure @romani ! I will provide you complete POC/manual on your repository/ related ticket asap.
> If you can enforce, no checkstyle violation in CI, and make it possible to change checkstyle version to any version from CLI arguments, checkstyle team can add Kafka project to regression suite and you will never have false positives from CheckStyle any more, as we will validate Kafka execution on each commit in checkstyle.
I am only a (modest) apache-kafka contributor, but I do know that @ijuma can answer you about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] romani edited a comment on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
romani edited a comment on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835809154
@dejan2609, can you share she'll command that checkstyle team can execute to run only checkstyle validation, we will make sure no false positives happen.
Please share link to checkstyle config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] romani commented on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
romani commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-840296162
When you finish build config update to define checkstyle version from CLI, please share with us command.
We will do
```
git clone
Git checkout ...
Your build command
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-836071563
> comply with these new rules and change affected classes (note: total of 50 violations in 18 classes is recorded when compiled with Checkstyle 8.41.1)
Can you provide a bit more detail on the stricter rules?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 edited a comment on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 edited a comment on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835815883
> @dejan2609, can you share she'll command that checkstyle team can execute to run only checkstyle validation, we will make sure no false positives happen.
> Please share link to checkstyle config.
Sure @romani ! I will provide you complete POC/manual on your repository / related ticket asap.
> If you can enforce, no checkstyle violation in CI, and make it possible to change checkstyle version to any version from CLI arguments, checkstyle team can add Kafka project to regression suite and you will never have false positives from CheckStyle any more, as we will validate Kafka execution on each commit in checkstyle.
Kafka is using Jenkins: https://github.com/apache/kafka/blob/2.8.0/Jenkinsfile#L28; it should not be that hard to implement your requirements (I did managed to pull out similar things on Jenkins) but I really can't speak for Apache Kafka project because I am only external contributor).
Kafka project maintainers should be involved here, I guess. FYI @mumrah and @ijuma
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 commented on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-840906914
Sure @romani ! Hopefully I will have something for you in a day or two.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma merged pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #10656:
URL: https://github.com/apache/kafka/pull/10656
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-836074067
> If you can enforce, no checkstyle violation in CI, and make it possible to change checkstyle version to any version from CLI arguments, checkstyle team can add Kafka project to regression suite and you will never have false positives from CheckStyle any more, as we will validate Kafka execution on each commit in checkstyle.
@romani sure, we can do that. @dejan2609 Do you want to contribute the change to allow the checkstyle version to be specified via a parameter? We have a few examples here https://github.com/apache/kafka#common-build-options
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 edited a comment on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 edited a comment on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835815883
> @dejan2609, can you share she'll command that checkstyle team can execute to run only checkstyle validation, we will make sure no false positives happen.
> Please share link to checkstyle config.
Sure @romani ! I will provide you complete POC/manual on your repository / related ticket asap.
> If you can enforce, no checkstyle violation in CI, and make it possible to change checkstyle version to any version from CLI arguments, checkstyle team can add Kafka project to regression suite and you will never have false positives from CheckStyle any more, as we will validate Kafka execution on each commit in checkstyle.
I am only a (modest) apache-kafka contributor, but I do know that @ijuma can answer you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-836063577
One flaky test for jdk 11/scala 2.13 build, the rest passed:
> Build / JDK 11 and Scala 2.13 / kafka.network.ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 commented on pull request #10656: MINOR: code quality check tools (checkstyle and spotbugs): version upgrades
Posted by GitBox <gi...@apache.org>.
dejan2609 commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835792416
My bad... I was rushing to scrap PR so I just didn't check spotbugs related gradle tasks on my end... I was counting on a fact that this is just patch, but it turns our that spotbugs version 4.2.3 is not stable. So, it is just checkstyle upgrade, after all.
@ijuma feel free to review now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 commented on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-837469070
@romani, @ijuma
> Do you want to contribute the change to allow the checkstyle version to be specified via a parameter?
Sure, I created this JIRA ticket for my self (PR will follow in next few days, I hope): https://issues.apache.org/jira/browse/KAFKA-12770 **_Jenkins build: allow the CheckStyle version to be specified via parameter_**
> Can you provide a bit more detail on the stricter rules?
Separate ticket is filed here: https://issues.apache.org/jira/browse/KAFKA-12771 **_CheckStyle attempted upgrade (8.36.2 -->> 8.41.1) summons a pack of 'Indentation' errors_**
I will provide some more details for discussion soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 commented on pull request #10656: MINOR: code quality check tools (checkstyle and spotbugs): version upgrades
Posted by GitBox <gi...@apache.org>.
dejan2609 commented on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835786890
Alas, spotbugs checks fail... https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-10656/1/pipeline
Let me check again on mu end... converting to a draft.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dejan2609 edited a comment on pull request #10656: MINOR: checkstyle version upgrade: 8.20 -->> 8.36.2
Posted by GitBox <gi...@apache.org>.
dejan2609 edited a comment on pull request #10656:
URL: https://github.com/apache/kafka/pull/10656#issuecomment-835792416
My bad... I was cutting corners while creating this PR, so I just didn't check spotbugs related gradle tasks on my end... I was counting on a fact that this is just patch, but it turns our that spotbugs version 4.2.3 is not stable.
So, it is just checkstyle upgrade, after all.
@ijuma feel free to review now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org