Repository: accumulo Updated Branches: refs/heads/master a12b3741a -> 739f906cf
ACCUMULO-4113 Fix incorrect usage of ByteBuffer Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/27300d81 Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/27300d81 Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/27300d81 Branch: refs/heads/master Commit: 27300d81eaee14516e705d0032bc0bdc46880d4c Parents: a2c2d38 Author: Keith Turner <ktur...@apache.org> Authored: Wed Jan 20 12:36:20 2016 -0500 Committer: Keith Turner <ktur...@apache.org> Committed: Wed Jan 20 12:36:20 2016 -0500 ---------------------------------------------------------------------- .../accumulo/core/data/ArrayByteSequence.java | 12 ++- .../accumulo/core/util/ByteBufferUtil.java | 34 +++++-- .../core/util/UnsynchronizedBuffer.java | 4 +- .../accumulo/core/util/ByteBufferUtilTest.java | 98 ++++++++++++++++++++ 4 files changed, 132 insertions(+), 16 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/27300d81/core/src/main/java/org/apache/accumulo/core/data/ArrayByteSequence.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/data/ArrayByteSequence.java b/core/src/main/java/org/apache/accumulo/core/data/ArrayByteSequence.java index d9ddc67..d040139 100644 --- a/core/src/main/java/org/apache/accumulo/core/data/ArrayByteSequence.java +++ b/core/src/main/java/org/apache/accumulo/core/data/ArrayByteSequence.java @@ -21,6 +21,8 @@ import static com.google.common.base.Charsets.UTF_8; import java.io.Serializable; import java.nio.ByteBuffer; +import org.apache.accumulo.core.util.ByteBufferUtil; + public class ArrayByteSequence extends ByteSequence implements Serializable { private static final long serialVersionUID = 1L; @@ -52,15 +54,14 @@ public class ArrayByteSequence extends ByteSequence implements Serializable { } public ArrayByteSequence(ByteBuffer buffer) { - this.length = buffer.remaining(); - if (buffer.hasArray()) { this.data = buffer.array(); - this.offset = buffer.position(); + this.offset = buffer.position() + buffer.arrayOffset(); + this.length = buffer.remaining(); } else { - this.data = new byte[length]; this.offset = 0; - buffer.get(data); + this.data = ByteBufferUtil.toBytes(buffer); + this.length = data.length; } } @@ -118,6 +119,7 @@ public class ArrayByteSequence extends ByteSequence implements Serializable { return copy; } + @Override public String toString() { return new String(data, offset, length, UTF_8); } http://git-wip-us.apache.org/repos/asf/accumulo/blob/27300d81/core/src/main/java/org/apache/accumulo/core/util/ByteBufferUtil.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/util/ByteBufferUtil.java b/core/src/main/java/org/apache/accumulo/core/util/ByteBufferUtil.java index be5cddf..cdde05c 100644 --- a/core/src/main/java/org/apache/accumulo/core/util/ByteBufferUtil.java +++ b/core/src/main/java/org/apache/accumulo/core/util/ByteBufferUtil.java @@ -31,7 +31,15 @@ public class ByteBufferUtil { public static byte[] toBytes(ByteBuffer buffer) { if (buffer == null) return null; - return Arrays.copyOfRange(buffer.array(), buffer.position(), buffer.limit()); + if (buffer.hasArray()) { + // did not use buffer.get() because it changes the position + return Arrays.copyOfRange(buffer.array(), buffer.position() + buffer.arrayOffset(), buffer.limit() + buffer.arrayOffset()); + } else { + byte[] data = new byte[buffer.remaining()]; + // duplicate inorder to avoid changing position + buffer.duplicate().get(data); + return data; + } } public static List<ByteBuffer> toByteBuffers(Collection<byte[]> bytesList) { @@ -47,23 +55,32 @@ public class ByteBufferUtil { public static List<byte[]> toBytesList(Collection<ByteBuffer> bytesList) { if (bytesList == null) return null; - ArrayList<byte[]> result = new ArrayList<byte[]>(); + ArrayList<byte[]> result = new ArrayList<byte[]>(bytesList.size()); for (ByteBuffer bytes : bytesList) { result.add(toBytes(bytes)); } return result; } - public static Text toText(ByteBuffer bytes) { - if (bytes == null) + public static Text toText(ByteBuffer byteBuffer) { + if (byteBuffer == null) return null; - Text result = new Text(); - result.set(bytes.array(), bytes.position(), bytes.remaining()); - return result; + + if (byteBuffer.hasArray()) { + Text result = new Text(); + result.set(byteBuffer.array(), byteBuffer.arrayOffset() + byteBuffer.position(), byteBuffer.remaining()); + return result; + } else { + return new Text(toBytes(byteBuffer)); + } } public static String toString(ByteBuffer bytes) { - return new String(bytes.array(), bytes.position(), bytes.remaining(), UTF_8); + if (bytes.hasArray()) { + return new String(bytes.array(), bytes.arrayOffset() + bytes.position(), bytes.remaining(), UTF_8); + } else { + return new String(toBytes(bytes), UTF_8); + } } public static ByteBuffer toByteBuffers(ByteSequence bs) { @@ -73,7 +90,6 @@ public class ByteBufferUtil { if (bs.isBackedByArray()) { return ByteBuffer.wrap(bs.getBackingArray(), bs.offset(), bs.length()); } else { - // TODO create more efficient impl return ByteBuffer.wrap(bs.toArray()); } } http://git-wip-us.apache.org/repos/asf/accumulo/blob/27300d81/core/src/main/java/org/apache/accumulo/core/util/UnsynchronizedBuffer.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/util/UnsynchronizedBuffer.java b/core/src/main/java/org/apache/accumulo/core/util/UnsynchronizedBuffer.java index f353613..b776553 100644 --- a/core/src/main/java/org/apache/accumulo/core/util/UnsynchronizedBuffer.java +++ b/core/src/main/java/org/apache/accumulo/core/util/UnsynchronizedBuffer.java @@ -122,8 +122,8 @@ public class UnsynchronizedBuffer { offset = buffer.arrayOffset() + buffer.position(); data = buffer.array(); } else { - data = new byte[buffer.remaining()]; - buffer.get(data); + offset = 0; + data = ByteBufferUtil.toBytes(buffer); } } http://git-wip-us.apache.org/repos/asf/accumulo/blob/27300d81/core/src/test/java/org/apache/accumulo/core/util/ByteBufferUtilTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/accumulo/core/util/ByteBufferUtilTest.java b/core/src/test/java/org/apache/accumulo/core/util/ByteBufferUtilTest.java new file mode 100644 index 0000000..f4ca0e9 --- /dev/null +++ b/core/src/test/java/org/apache/accumulo/core/util/ByteBufferUtilTest.java @@ -0,0 +1,98 @@ +/* + * 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.accumulo.core.util; + +import java.nio.ByteBuffer; +import java.util.Collections; +import java.util.List; + +import org.apache.accumulo.core.data.ArrayByteSequence; +import org.apache.hadoop.io.Text; +import org.junit.Assert; +import org.junit.Test; + +import com.google.common.base.Charsets; + +public class ByteBufferUtilTest { + + private static void assertEquals(String expected, ByteBuffer bb) { + Assert.assertEquals(new Text(expected), ByteBufferUtil.toText(bb)); + Assert.assertEquals(expected, new String(ByteBufferUtil.toBytes(bb), Charsets.UTF_8)); + Assert.assertEquals(expected, ByteBufferUtil.toString(bb)); + + List<byte[]> bal = ByteBufferUtil.toBytesList(Collections.singletonList(bb)); + Assert.assertEquals(1, bal.size()); + Assert.assertEquals(expected, new String(bal.get(0), Charsets.UTF_8)); + + Assert.assertEquals(new ArrayByteSequence(expected), new ArrayByteSequence(bb)); + } + + @Test + public void testNonZeroArrayOffset() { + byte[] data = "0123456789".getBytes(Charsets.UTF_8); + + ByteBuffer bb1 = ByteBuffer.wrap(data, 3, 4); + + // create a ByteBuffer with a non-zero array offset + ByteBuffer bb2 = bb1.slice(); + + // The purpose of this test is to ensure ByteBufferUtil code works when arrayOffset is non-zero. The following asserts are not to test ByteBuffer, but + // ensure the behavior of slice() is as expected. + + Assert.assertEquals(3, bb2.arrayOffset()); + Assert.assertEquals(0, bb2.position()); + Assert.assertEquals(4, bb2.limit()); + + // start test with non zero arrayOffset + assertEquals("3456", bb2); + + // read one byte from byte buffer... this should cause position to be non-zero in addition to array offset + bb2.get(); + assertEquals("456", bb2); + + } + + @Test + public void testZeroArrayOffsetAndNonZeroPosition() { + byte[] data = "0123456789".getBytes(Charsets.UTF_8); + ByteBuffer bb1 = ByteBuffer.wrap(data, 3, 4); + + assertEquals("3456", bb1); + } + + @Test + public void testZeroArrayOffsetAndPosition() { + byte[] data = "0123456789".getBytes(Charsets.UTF_8); + ByteBuffer bb1 = ByteBuffer.wrap(data, 0, 4); + assertEquals("0123", bb1); + } + + @Test + public void testDirectByteBuffer() { + // allocate direct so it does not have a backing array + ByteBuffer bb = ByteBuffer.allocateDirect(10); + bb.put("0123456789".getBytes(Charsets.UTF_8)); + bb.rewind(); + + assertEquals("0123456789", bb); + + // advance byte buffer position + bb.get(); + assertEquals("123456789", bb); + } +}
