Skip to content

fixes based on static analysis check with FindBugs#5842

Merged
orpiske merged 8 commits intoapache:mainfrom
dk2k:dk2k-components-static-analysis
Jul 21, 2021
Merged

fixes based on static analysis check with FindBugs#5842
orpiske merged 8 commits intoapache:mainfrom
dk2k:dk2k-components-static-analysis

Conversation

@dk2k
Copy link
Contributor

@dk2k dk2k commented Jul 17, 2021

No description provided.

@dk2k dk2k marked this pull request as draft July 18, 2021 06:51
@dk2k dk2k marked this pull request as ready for review July 18, 2021 06:51
Copy link
Contributor

@orpiske orpiske left a comment

Choose a reason for hiding this comment

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

Thanks for helping keeping the code code tidy and clean! I added a few notes about the log messages, as it would make it easier for users to figure out what went wrong should they ever happen.

@dk2k dk2k requested a review from orpiske July 19, 2021 16:56
Copy link
Contributor

@orpiske orpiske left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! It's looking better, but the usage of log markers is incorrect. I added some examples, but there may be more.

@dk2k dk2k requested a review from orpiske July 20, 2021 10:55
Copy link
Contributor

@orpiske orpiske left a comment

Choose a reason for hiding this comment

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

This is looking great, thanks for the changes! I think the only remaining items I see are the commented blocks. Once removing that, we should be good to go.

// prefer to use endpoint configured over component configured
if (jettyBinding == null) {
// unused
/*if (jettyBinding == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this block of code if it's unused. It would make it easier for us when/if bisecting the code in case of problems.

@dk2k dk2k requested a review from orpiske July 20, 2021 17:08
Copy link
Contributor

@orpiske orpiske 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 for going through the requested changes @dk2k.

@orpiske orpiske merged commit abefdfd into apache:main Jul 21, 2021
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