KAFKA-4796: Fix some findbugs warnings in Kafka Java client #2593
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| @@ -68,7 +68,7 @@ | |||
| */ | |||
| public class ConfigDef { | |||
|
|
|||
| public static final Object NO_DEFAULT_VALUE = new String(""); | |||
| public static final Object NO_DEFAULT_VALUE = ""; | |||
There was a problem hiding this comment.
This change is not right. We are using this field as a sentinel and that's why there is the new String there. We could perhaps just use new Object as this seems to confuse findBugs and people (it's not the first time that someone thought this was a bug).
There was a problem hiding this comment.
Thanks for the explanation. Let me add a comment explaining why the new is there.
| if (thisField == null) | ||
| return otherField == null; | ||
| else | ||
| return thisField.equals(otherField); |
There was a problem hiding this comment.
The ternary operator is often used in such cases, any reason not to do that here?
result = thisField == null ? otherField == null : thisFied.equals(otherField);There was a problem hiding this comment.
Ternary sounds good here
| @@ -139,7 +140,7 @@ private static String toString(final byte[] b, int off, int len) { | |||
| */ | |||
| public final static Comparator<byte[]> BYTES_LEXICO_COMPARATOR = new LexicographicByteArrayComparator(); | |||
|
|
|||
| private interface ByteArrayComparator extends Comparator<byte[]> { | |||
| private interface ByteArrayComparator extends Comparator<byte[]>, Serializable { | |||
There was a problem hiding this comment.
We normally don't make things Serializable unless there's a good reason to do it. Is there a good reason?
There was a problem hiding this comment.
The JavaDoc for java.util.Comparator says that:
Note: It is generally a good idea for comparators to also implement java.io.Serializable, as they may be used as ordering methods in serializable data structures (like TreeSet, TreeMap). In order for the data structure to serialize successfully, the comparator (if provided) must implement Serializable.
(see http://docs.oracle.com/javase/7/docs/api/java/util/Comparator.html)
There seems to be no harm in implementing Serializable here since the Comparator has no internal state, so there's nothing to do.
| @@ -126,7 +126,7 @@ public static long readUnsignedInt(ByteBuffer buffer, int index) { | |||
| * @return The integer read (MUST BE TREATED WITH SPECIAL CARE TO AVOID SIGNEDNESS) | |||
| */ | |||
| public static int readUnsignedIntLE(InputStream in) throws IOException { | |||
| return (in.read() << 8 * 0) | |||
| return (in.read() & 0xff) | |||
There was a problem hiding this comment.
I have been debating whether I should change this every time I saw it. :) I guess whoever wrote it liked the fact there was a regular pattern followed by each line.
There was a problem hiding this comment.
We have a bunch of other 8 * 0 in the file. Do we want to update those too?
There was a problem hiding this comment.
Looking at this closer, it seems to be incorrect...
| @@ -258,4 +259,15 @@ private void check(Type type, Object obj) { | |||
| assertEquals("The object read back should be the same as what was written.", obj, result); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testStructEquals() { | |||
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
@cmccabe, I added some tests, removed some unused (and untested) code and did a few tweaks: If it looks good to you, please cherry-pick to your branch and I'll merge this PR. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Thanks. I added the pull request to this patch series and rebased on trunk |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
No description provided.