Skip to content

KAFKA-13705: CoreUtils.swallow uses logging parameter instead of Logger #11841

Closed
Corlobin wants to merge 6 commits intoapache:trunkfrom
Corlobin:issues/13705
Closed

KAFKA-13705: CoreUtils.swallow uses logging parameter instead of Logger #11841
Corlobin wants to merge 6 commits intoapache:trunkfrom
Corlobin:issues/13705

Conversation

@Corlobin
Copy link
Copy Markdown

@Corlobin Corlobin commented Mar 3, 2022

As described in the issue, the:

  • Javadoc says: The logging instance to use for logging the thrown exception.

But logging parameter it is never used.

The testing strategy used was to run the CoreUtilsTest class and all test projects classes to check if any was broken.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Copy Markdown
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.

@Corlobin , thanks for the PR. LGTM. But there are spotBug error, could you fix them? Thanks.

@Corlobin Corlobin requested a review from showuon March 4, 2022 03:43
@Corlobin
Copy link
Copy Markdown
Author

Corlobin commented Mar 4, 2022

Hi Luke, thanks! I'm taking a look!

@Corlobin
Copy link
Copy Markdown
Author

Corlobin commented Mar 4, 2022

Hi @showuon, I believe that is a false-positive, running the spotbugsMain from trunk I receive 6 warnings as described below: spotbugs/spotbugs#1963

I reported a bug on spotbugs and commited the ignore file on spotbugs-exclude.xml.

I'm waiting a answer from spotbugs repo but I believe ithis s the same false-positive from others spotbugs described in the spotbugs-exclude file.

Copy link
Copy Markdown
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.

Comment thread gradle/spotbugs-exclude.xml Outdated
some categories of bug reports when using Scala, since spotbugs generates huge
numbers of false positives in some of these categories when examining Scala code.

SA_FIELD_SELF_COMPARISON: This method compares a field with itself, and may indicate a typo or a logic error.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the investigation. I agree it should be false positive. Could we add the spotbugs github issue link here to indicate this should be removed after issue fixed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks @showuon , great idea, github issue link added!

@Corlobin
Copy link
Copy Markdown
Author

Corlobin commented Mar 6, 2022

Thanks @showuon it seems these tests failed appeared on merge of another PR: #11830 after my investigation.

Excluded line: https://github.com/apache/kafka/pull/11830/files#r820125170 (as commented)

Link error with these tests: https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-11830/3/tests

Existing failures - 10
Build / JDK 8 and Scala 2.12 / testSendWithInvalidCreateTime() – kafka.api.PlaintextProducerSendTest
3s
Build / JDK 8 and Scala 2.12 / testSendWithInvalidCreateTime() – kafka.api.PlaintextProducerSendTest
6s
Build / JDK 8 and Scala 2.12 / testProduceWithInvalidTimestamp() – kafka.server.ProduceRequestTest
2s
Build / JDK 8 and Scala 2.12 / testProduceWithInvalidTimestamp() – kafka.server.ProduceRequestTest
2s
Build / JDK 11 and Scala 2.13 / testSendWithInvalidCreateTime() – kafka.api.PlaintextProducerSendTest
2s
Build / JDK 11 and Scala 2.13 / testSendWithInvalidCreateTime() – kafka.api.PlaintextProducerSendTest
7s
Build / JDK 11 and Scala 2.13 / testProduceWithInvalidTimestamp() – kafka.server.ProduceRequestTest
3s
Build / JDK 11 and Scala 2.13 / testProduceWithInvalidTimestamp() – kafka.server.ProduceRequestTest
2s
Build / JDK 17 and Scala 2.13 / testSendWithInvalidCreateTime() – kafka.api.PlaintextProducerSendTest
3s
Build / JDK 17 and Scala 2.13 / testProduceWithInvalidTimestamp() – kafka.server.ProduceRequestTest
3s

Could you confirm please if was introduced by my PR or was failing before?

@Corlobin Corlobin requested a review from showuon March 6, 2022 02:16
@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 6, 2022

Yes, you're right! Those tests are introduced by other PR. However, the recent jenkins build doesn't complete successfully. I've triggered another one to check it.
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-11841/7/

@Corlobin
Copy link
Copy Markdown
Author

Corlobin commented Mar 6, 2022

Hi @showuon , thanks!!!

It seems the error is related to a failed gradle step not related to my commits.
image

As you can see build passes:
image

I Investigated and I do believe these errors are related to this line:

junit '**/build/test-results/**/TEST-*.xml'

I'll try to keep investigating.. maybe this is due to failed tests? (related to my previous comment)

@github-actions
Copy link
Copy Markdown

This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

@github-actions github-actions bot added the stale Stale PRs label Dec 26, 2024
@github-actions
Copy link
Copy Markdown

This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.

@github-actions github-actions bot added the closed-stale PRs that were closed due to inactivity label Jan 26, 2025
@github-actions github-actions bot closed this Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

closed-stale PRs that were closed due to inactivity stale Stale PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants