Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,12 @@ matrix:
install: true
script: MAVEN_OPTS='-Xmx3000m' mvn clean verify -Prat -DskipTests -B -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn -Drat.consoleOutput=true

# strict compilation
# strict compilation and future guava compat
- env:
- NAME="strict compilation"
- NAME="strict compilation and guava checks"
install: true
# Strict compilation requires more than 2 GB
script: MAVEN_OPTS='-Xmx3000m' mvn clean -Pstrict -pl '!benchmarks' compile test-compile -B --fail-at-end
script: MAVEN_OPTS='-Xmx3000m' mvn clean -Pstrict -Precent-guava -pl '!benchmarks' compile test-compile -B --fail-at-end

# processing module test
- env:
Expand Down
7 changes: 6 additions & 1 deletion codestyle/druid-forbidden-apis.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,12 @@ java.util.Random#<init>() @ Use ThreadLocalRandom.current() or the constructor w
java.lang.Math#random() @ Use ThreadLocalRandom.current()
java.util.regex.Pattern#matches(java.lang.String,java.lang.CharSequence) @ Use String.startsWith(), endsWith(), contains(), or compile and cache a Pattern explicitly
org.apache.commons.io.FileUtils#getTempDirectory() @ Use org.junit.rules.TemporaryFolder for tests instead

com.google.common.net.HostAndPort#getHostText() @ Use org.apache.druid.common.guava.GuavaUtils#getHostText instead
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.

Would be nice to break this file into sections: jackson, guava, jdk, everything else.

com.google.common.base.CharMatcher.BREAKING_WHITESPACE @ Use org.apache.druid.common.guava.GuavaUtils#breakingWhitespace()
com.google.common.base.CharMatcher#breakingWhitespace() @ Use org.apache.druid.common.guava.GuavaUtils#breakingWhitespace()
com.google.common.base.Objects#toStringHelper(java.lang.Object) @ Deprecated in future guava
com.google.common.base.Objects#toStringHelper(java.lang.Class<?>) @ Deprecated in future guava
com.google.common.base.Objects#toStringHelper(java.lang.String) @ Deprecated in future guava
@defaultMessage For performance reasons, use the utf8Base64 / encodeBase64 / encodeBase64String / decodeBase64 / decodeBase64String methods in StringUtils
org.apache.commons.codec.binary.Base64
com.google.common.io.BaseEncoding.base64
110 changes: 110 additions & 0 deletions core/src/main/java/org/apache/druid/common/guava/GuavaUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,58 @@

package org.apache.druid.common.guava;

import com.google.common.base.CharMatcher;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.net.HostAndPort;
import com.google.common.primitives.Longs;
import org.apache.druid.java.util.common.IAE;
import org.apache.druid.java.util.common.StringUtils;

import javax.annotation.Nullable;
import javax.el.MethodNotFoundException;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Arrays;

/**
* This class contains a bunch of guava helper functions to bridge compatability problems across guava versions, or
* between guava and java standard libraries. Any version compatability related entries should have a reference to
* the critical commit(s).
*/
public class GuavaUtils
{
private static final CharMatcher BREAKING_WHITESPACE_INSTANCE;

static {
// https://github.com/google/guava/commit/4fbb165ebf208d75100d5d47f56750d247f7d181
// Since v19.0
CharMatcher matcher;
try {
final Method m = CharMatcher.class.getDeclaredMethod("breakingWhitespace");
matcher = (CharMatcher) m.invoke(null);
}
catch (IllegalAccessException | InvocationTargetException e) {
throw new IllegalStateException("Failed to fetch breakingWhitespace", e);
}
catch (NoSuchMethodException e) {
try {
final Field f = CharMatcher.class.getDeclaredField("BREAKING_WHITESPACE");
matcher = (CharMatcher) f.get(null);
}
catch (IllegalAccessException e1) {
throw new IllegalStateException("Failed to access BREAKING_WHITESPACE", e1);
}
catch (NoSuchFieldException e1) {
throw new IllegalStateException("Cannot find breaking white space in guava", e1);
}
}
if (matcher == null) {
throw new IllegalStateException("wtf!?");
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.

Please make the error message more constructive, or absent altogether

}
BREAKING_WHITESPACE_INSTANCE = matcher;
}

/**
* To fix semantic difference of Longs.tryParse() from Long.parseLong (Longs.tryParse() returns null for '+' started value)
Expand Down Expand Up @@ -62,4 +104,72 @@ public static <T extends Enum<T>> T getEnumIfPresent(final Class<T> enumClass, f

return null;
}

/**
* Try the various methods (zero arguments) against the object. This is handy for maintaining guava compatability
*
* @param object The object to call the methods on
* @param methods The sequence of methods to call
* @param <T> The return type
*
* @return The result of invoking the method on the object
*/
private static <T> T tryMethods(Object object, Class<T> assignableTo, String... methods)
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.

Documentation should reference the specific commit and the range of lines (as a Github link) in Guava from where this was copied. Same for other non-trivial parts of the copied code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This part was not copied. The method names in the callers are copied though. I'll add a javadoc here to help make sure a "use the git tag as a reference" pattern is followed in the future for such behaviors.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

{
for (String method : methods) {
try {
final Method m = object.getClass().getDeclaredMethod(method);
if (!assignableTo.isAssignableFrom(m.getReturnType())) {
throw new IAE(
"Cannot assign [%s] to [%s] in [%s] from [%s]",
m.getReturnType(),
assignableTo,
m,
object.getClass()
);
}
try {
return (T) m.invoke(object);
}
catch (IllegalAccessException | InvocationTargetException e) {
throw new IAE("Failed to invoke [%s] on [%s]", m, object);
}
}
catch (NoSuchMethodException e) {
// Keep going
}
}
throw new MethodNotFoundException(StringUtils.format(
"Unable to find methods %s in [%s]",
Arrays.toString(methods),
object.getClass()
));
}

/**
* Get the host portion of the {@link HostAndPort} that has the host's text. Changes in different guava versions
*
* @param hostAndPort The object to pull from
*
* @return The host portion of the host and port
*/
public static String getHostText(HostAndPort hostAndPort)
{
// https://github.com/google/guava/commit/b0babb69b05ed4d15cce74635ae96cf8ba78c85f
// Change started in v20.0
return tryMethods(hostAndPort, String.class, "getHostText", "getHost");
}

/**
* Returns the instance of the breaking whitespace char matcher in guava.
*
* This moved starting in guava 19
*
* @return `CharMatcher.BREAKING_WHITESPACE` or `CharMatcher.breakingWhitespace()` depending on whichever works
*/

public static CharMatcher breakingWhitespace()
{
return BREAKING_WHITESPACE_INSTANCE;
}
}
23 changes: 12 additions & 11 deletions core/src/main/java/org/apache/druid/indexer/TaskStatus.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Objects;
import com.google.common.base.Preconditions;

import java.util.Objects;

/**
* Represents the status of a task from the perspective of the coordinator. The task may be ongoing
* ({@link #isComplete()} false) or it may be complete ({@link #isComplete()} true).
Expand Down Expand Up @@ -178,12 +179,12 @@ public TaskStatus withDuration(long _duration)
@Override
public String toString()
{
return Objects.toStringHelper(this)
.add("id", id)
.add("status", status)
.add("duration", duration)
.add("errorMsg", errorMsg)
.toString();
return "TaskStatus{" +
"id='" + id + '\'' +
", status=" + status +
", duration=" + duration +
", errorMsg='" + errorMsg + '\'' +
'}';
}

@Override
Expand All @@ -196,15 +197,15 @@ public boolean equals(Object o)
return false;
}
TaskStatus that = (TaskStatus) o;
return getDuration() == that.getDuration() &&
java.util.Objects.equals(getId(), that.getId()) &&
return duration == that.duration &&
id.equals(that.id) &&
status == that.status &&
java.util.Objects.equals(getErrorMsg(), that.getErrorMsg());
Objects.equals(errorMsg, that.errorMsg);
}

@Override
public int hashCode()
{
return java.util.Objects.hash(getId(), status, getDuration(), getErrorMsg());
return Objects.hash(id, status, duration, errorMsg);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,34 +42,42 @@ public static <I, O> ListenableFuture<O> transformAsync(
)
{
final SettableFuture<O> finalFuture = SettableFuture.create();
Futures.addCallback(inFuture, new FutureCallback<I>()
{
@Override
public void onSuccess(@Nullable I result)
{
final ListenableFuture<O> transformFuture = transform.apply(result);
Futures.addCallback(transformFuture, new FutureCallback<O>()
Futures.addCallback(
inFuture,
new FutureCallback<I>()
{
@Override
public void onSuccess(@Nullable O result)
public void onSuccess(@Nullable I result)
{
finalFuture.set(result);
final ListenableFuture<O> transformFuture = transform.apply(result);
Futures.addCallback(
transformFuture,
new FutureCallback<O>()
{
@Override
public void onSuccess(@Nullable O result)
{
finalFuture.set(result);
}

@Override
public void onFailure(Throwable t)
{
finalFuture.setException(t);
}
},
Runnable::run
);
}

@Override
public void onFailure(Throwable t)
{
finalFuture.setException(t);
}
});
}

@Override
public void onFailure(Throwable t)
{
finalFuture.setException(t);
}
});
},
Runnable::run
);
return finalFuture;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,13 @@

package org.apache.druid.common.guava;

import com.google.common.net.HostAndPort;
import com.google.common.primitives.Longs;
import org.junit.Assert;
import org.junit.Test;

public class GuavaUtilsTest
{
enum MyEnum
{
ONE,
TWO,
BUCKLE_MY_SHOE
}

@Test
public void testParseLong()
{
Expand All @@ -53,4 +47,25 @@ public void testGetEnumIfPresent()
Assert.assertEquals(MyEnum.BUCKLE_MY_SHOE, GuavaUtils.getEnumIfPresent(MyEnum.class, "BUCKLE_MY_SHOE"));
Assert.assertEquals(null, GuavaUtils.getEnumIfPresent(MyEnum.class, "buckle_my_shoe"));
}

@Test
public void testHostAndPorthostText()
{
Assert.assertEquals("localhost", GuavaUtils.getHostText(HostAndPort.fromString("localhost")));
Assert.assertEquals("127.0.0.1", GuavaUtils.getHostText(HostAndPort.fromString("127.0.0.1")));
Assert.assertEquals("::1", GuavaUtils.getHostText(HostAndPort.fromString("::1")));
}

@Test
public void testBreakingWhitespaceExists()
{
Assert.assertNotNull(GuavaUtils.breakingWhitespace());
}

enum MyEnum
{
ONE,
TWO,
BUCKLE_MY_SHOE
}
}
Loading