Skip to content
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ public void setup()
null,
null,
null,
null,
null
)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,41 @@
* under the License.
*/

package org.apache.druid.indexer.partitions;
package org.apache.druid.indexer;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Package for ChecksTest needs to be updated similarly


import org.apache.druid.java.util.common.IAE;

import java.util.List;

/**
* Various helper methods useful for checking the validity of arguments to spec constructors.
*/
class Checks
public final class Checks
{
public static <T> Property<T> checkOneNotNullOrEmpty(List<Property<T>> properties)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add relevant tests to ChecksTest

{
Property<T> nonNullProperty = null;
for (Property<T> property : properties) {
if (!property.isValueNullOrEmptyCollection()) {
if (nonNullProperty == null) {
nonNullProperty = property;
} else {
throw new IAE("At most one of %s must be present", properties);
}
}
}
if (nonNullProperty == null) {
throw new IAE("At most one of %s must be present", properties);
}
return nonNullProperty;
}

/**
* @return Non-null value, or first one if both are null. -1 is interpreted as null for historical reasons.
*/
@SuppressWarnings("VariableNotUsedInsideIf") // false positive: checked for 'null' not used inside 'if
static Property<Integer> checkAtMostOneNotNull(Property<Integer> property1, Property<Integer> property2)
public static <T> Property<T> checkAtMostOneNotNull(Property<T> property1, Property<T> property2)
{
final Property<Integer> property;
final Property<T> property;

boolean isNull1 = property1.getValue() == null;
boolean isNull2 = property2.getValue() == null;
Expand All @@ -42,9 +63,7 @@ static Property<Integer> checkAtMostOneNotNull(Property<Integer> property1, Prop
} else if (isNull2) {
property = property1;
} else {
throw new IllegalArgumentException(
"At most one of " + property1.getName() + " or " + property2.getName() + " must be present"
);
throw new IAE("At most one of [%s] or [%s] must be present", property1, property2);
}

return property;
Expand All @@ -53,10 +72,14 @@ static Property<Integer> checkAtMostOneNotNull(Property<Integer> property1, Prop
/**
* @return Non-null value, or first one if both are null. -1 is interpreted as null for historical reasons.
*/
static Property<Integer> checkAtMostOneNotNull(String name1, Integer value1, String name2, Integer value2)
public static <T> Property<T> checkAtMostOneNotNull(String name1, T value1, String name2, T value2)
{
Property<Integer> property1 = new Property<>(name1, value1);
Property<Integer> property2 = new Property<>(name2, value2);
Property<T> property1 = new Property<>(name1, value1);
Property<T> property2 = new Property<>(name2, value2);
return checkAtMostOneNotNull(property1, property2);
}

private Checks()
{
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,20 @@
* under the License.
*/

package org.apache.druid.indexer.partitions;
package org.apache.druid.indexer;

import java.util.Collection;
import java.util.Objects;

/**
* Convenience class for holding a pair of string key and templated value.
*/
class Property<T>
public class Property<T>
{
private final String name;
private final T value;

Property(String name, T value)
public Property(String name, T value)
{
this.name = name;
this.value = value;
Expand All @@ -45,6 +46,17 @@ public T getValue()
return value;
}

public boolean isValueNullOrEmptyCollection()
{
if (value == null) {
return true;
}
if (value instanceof Collection) {
return ((Collection) value).isEmpty();
}
return false;
}

@Override
public boolean equals(Object o)
{
Expand All @@ -64,4 +76,13 @@ public int hashCode()
{
return Objects.hash(name, value);
}

@Override
public String toString()
{
return "Property{" +
"name='" + name + '\'' +
", value=" + value +
'}';
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import org.apache.druid.indexer.Checks;
import org.apache.druid.indexer.Property;

import javax.annotation.Nullable;
import java.util.Collections;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import org.apache.druid.indexer.Checks;
import org.apache.druid.indexer.Property;

import javax.annotation.Nullable;
import javax.validation.constraints.NotNull;
Expand Down
25 changes: 25 additions & 0 deletions core/src/main/java/org/apache/druid/segment/SegmentUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,43 @@

package org.apache.druid.segment;

import com.google.common.hash.HashFunction;
import com.google.common.hash.Hasher;
import com.google.common.hash.Hashing;
import com.google.common.io.Files;
import com.google.common.primitives.Ints;
import org.apache.druid.guice.annotations.PublicApi;
import org.apache.druid.java.util.common.IOE;
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.timeline.DataSegment;

import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.List;

/**
* Utility methods useful for implementing deep storage extensions.
*/
@PublicApi
public class SegmentUtils
{
private static final HashFunction HASH_FUNCTION = Hashing.sha256();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the hash does not need to be cryptographically secure, perhaps Hashing.murmur3_128() is a better option.

(If you change the hash function, the comment on 49 and CompactionIntervalSpec need to be updated too.)

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.

Hmm, why is it better?

Copy link
Copy Markdown
Contributor

@ccaominh ccaominh Oct 3, 2019

Choose a reason for hiding this comment

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

Cryptographically-secure hash functions are typically slower since they need to more work to achieve that property. For example, this perf test observed sha256 to be about an order of magnitude slower than murmur3: https://rusty.ozlabs.org/?p=511


/**
* Hash the IDs of the given segments based on SHA-256 algorithm.
*/
public static String hashIds(List<DataSegment> segments)
{
Collections.sort(segments);
final Hasher hasher = HASH_FUNCTION.newHasher();
segments.forEach(segment -> hasher.putString(segment.getId().toString(), StandardCharsets.UTF_8));
return StringUtils.fromUtf8(hasher.hash().asBytes());
}

public static int getVersionFromDir(File inDir) throws IOException
{
File versionFile = new File(inDir, "version.bin");
Expand All @@ -53,4 +74,8 @@ public static int getVersionFromDir(File inDir) throws IOException

throw new IOE("Invalid segment dir [%s]. Can't find either of version.bin or index.drd.", inDir);
}

private SegmentUtils()
{
}
}
148 changes: 148 additions & 0 deletions core/src/test/java/org/apache/druid/indexer/ChecksTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.druid.indexer;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import org.apache.druid.java.util.common.StringUtils;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import java.util.Collections;
import java.util.List;

public class ChecksTest
{
private static final String NAME1 = "name1";
private static final Integer VALUE1 = 1;
private static final String NAME2 = "name2";
private static final Integer VALUE2 = 2;
private static final Integer NULL = null;

@Rule
public ExpectedException exception = ExpectedException.none();

@Test
public void checkAtMostOneNotNullFirstNull()
{
Property<Integer> result = Checks.checkAtMostOneNotNull(NAME1, NULL, NAME2, VALUE2);
Assert.assertEquals(NAME2, result.getName());
Assert.assertEquals(VALUE2, result.getValue());
}

@Test
public void checkAtMostOneNotNullSecondNull()
{
Property<Integer> result = Checks.checkAtMostOneNotNull(NAME1, VALUE1, NAME2, NULL);
Assert.assertEquals(NAME1, result.getName());
Assert.assertEquals(VALUE1, result.getValue());
}

@Test
public void checkAtMostOneNotNullBothNull()
{
Property<Integer> result = Checks.checkAtMostOneNotNull(NAME1, NULL, NAME2, NULL);
Assert.assertEquals(NAME1, result.getName());
Assert.assertEquals(NULL, result.getValue());
}

@Test
public void checkAtMostOneNotNullNeitherNull()
{
exception.expect(IllegalArgumentException.class);
exception.expectMessage(
StringUtils.format(
"[Property{name='%s', value=%s}] or [Property{name='%s', value=%s}]",
NAME1,
VALUE1,
NAME2,
VALUE2
)
);

Checks.checkAtMostOneNotNull(NAME1, VALUE1, NAME2, VALUE2);
}

@Test
public void testCheckOneNotNullOrEmpty()
{
final List<Property<Object>> properties = ImmutableList.of(
new Property<>("p1", null),
new Property<>("p2", 2),
new Property<>("p3", null),
new Property<>("p4", Collections.emptyList())
);
final Property<Object> property = Checks.checkOneNotNullOrEmpty(properties);
Assert.assertEquals(new Property<>("p2", 2), property);
}

@Test
public void testCheckOneNotNullOrEmptyWithTwoNonNulls()
{
final List<Property<Object>> properties = ImmutableList.of(
new Property<>("p1", null),
new Property<>("p2", 2),
new Property<>("p3", 3),
new Property<>("p4", Collections.emptyList())
);
exception.expect(IllegalArgumentException.class);
exception.expectMessage(
"At most one of [Property{name='p1', value=null}, Property{name='p2', value=2}, Property{name='p3', value=3}, "
+ "Property{name='p4', value=[]}] must be present"
);
Checks.checkOneNotNullOrEmpty(properties);
}

@Test
public void testCheckOneNotNullOrEmptyWithNonNullAndNonEmpty()
{
final List<Property<Object>> properties = ImmutableList.of(
new Property<>("p1", null),
new Property<>("p2", 2),
new Property<>("p3", null),
new Property<>("p4", Lists.newArrayList(1, 2))
);
exception.expect(IllegalArgumentException.class);
exception.expectMessage(
"At most one of [Property{name='p1', value=null}, Property{name='p2', value=2}, Property{name='p3', value=null}, "
+ "Property{name='p4', value=[1, 2]}] must be present"
);
Checks.checkOneNotNullOrEmpty(properties);
}

@Test
public void testCheckOneNotNullOrEmptyWithAllNulls()
{
final List<Property<Object>> properties = ImmutableList.of(
new Property<>("p1", null),
new Property<>("p2", null),
new Property<>("p3", null),
new Property<>("p4", null)
);
exception.expect(IllegalArgumentException.class);
exception.expectMessage(
"At most one of [Property{name='p1', value=null}, Property{name='p2', value=null}, "
+ "Property{name='p3', value=null}, Property{name='p4', value=null}] must be present"
);
Checks.checkOneNotNullOrEmpty(properties);
}
}
Loading