KAFKA-5772 Improve Util classes

[matzew]

Done for https://issues.apache.org/jira/browse/KAFKA-5772

Utils with static methods should not be instantiated, hence marking the classes final and adding a private constructor.

this is consistent w/ some of the Util classes, such as ByteUtils, see:

kafka/clients/src/main/java/org/apache/kafka/common/utils/ByteUtils.java

Lines 29 to 31 in d345d53

	public final class ByteUtils {
	private ByteUtils() {}

[asfgit]

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/5472/
Test PASSed (JDK 7 and Scala 2.11).

[asfgit]

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5457/
Test PASSed (JDK 8 and Scala 2.12).

[matzew]

@rhauch do you mind taking a look? Or do I need to also have a JIRA for this improvement?

[rhauch]

LGTM!

[rhauch]

We still need a JIRA issue, and the title of this PR should start with that issue number per the contributors guide. Other than that, the code changes look great.

[matzew]

@rhauch Thanks Randall 👍

I've created a JIRA and update the tilte of the PR 😃

[rhauch]

@matzew, the Apache JIRA should automatically update with a reference to this PR, but I don't see that happening. Perhaps remove the " -" just after the issue number in the PR title?

[rhauch]

LGTM!

[rhauch]

Actually, on second thought I'm not sure whether these utility methods are considered part of the public API, and if so whether a KIP would be required. The Apache Kafka project are very strict about ensuring backward compatibility, and technically removing the constructor for these classes breaks backward compatibility.

Need advice from one of the committers, such as @hachikuji or @ijuma.

[ijuma]

These are class are internal.

[matzew]

@ijuma @rhauch thanks for clarification

[matzew]

@rhauch is there still interest in merging this? Or should I just close it ?

[rhauch]

@ijuma any reason not to approve and merge this?

[rhauch]

retest this please.

[ijuma]

Is there a reason why this one is not final?

[matzew]

@ijuma there was no particular reason, added final keyword here to

[ijuma]

retest this please

[ijuma]

One issue with final classes is that they're harder to mock (i.e. PowerMock is needed). For internal classes, the benefit of making classes final is not as pronounced as for public classes (since we can make any changes anyway). Still, happy to merge the PR if the tests pass.

[matzew]

@ijuma @rhauch thanks for the feedback.

I've now also rebased my branch, based on upstream's trunk!

[rhauch]

I'm going to approve this on a technical basis: the PR is simple and minimal. I'll leave to @ijuma to decide whether it's a desirable change.

[matzew]

should I close it at this point? Or worth to merge ?

…

On Thu, Sep 14, 2017 at 11:43 PM, Randall Hauch ***@***.***> wrote: ***@***.**** approved this pull request. I'm going to approve this on a technical basis: the PR is simple and minimal. I'll leave to @ijuma <https://github.com/ijuma> to decide whether it's a desirable change. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3370 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJnzqMKSOIpyfNS5L6z1xVfhEnjWRsnks5siZ3mgaJpZM4N-LTL> .

-- Matthias Wessendorf blog: http://matthiaswessendorf.wordpress.com/ twitter: http://twitter.com/mwessendorf

[matzew]

@ijuma should I close it or will it be merged ? :-)

[hachikuji]

Thanks for the patch. LGTM.

[matzew]

Thanks 😸