sodonnel commented on a change in pull request #3094: URL: https://github.com/apache/ozone/pull/3094#discussion_r811940081
########## File path: hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyInputStreamEC.java ########## @@ -0,0 +1,124 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.hadoop.ozone.client.io; + +import org.apache.hadoop.hdds.client.BlockID; +import org.apache.hadoop.hdds.client.ECReplicationConfig; +import org.apache.hadoop.hdds.client.ReplicationConfig; +import org.apache.hadoop.hdds.protocol.DatanodeDetails; +import org.apache.hadoop.hdds.scm.pipeline.Pipeline; +import org.apache.hadoop.hdds.scm.pipeline.PipelineID; +import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo; +import org.junit.Assert; +import org.junit.Test; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; + +import static org.apache.hadoop.ozone.OzoneConsts.MB; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.*; + +/** + * Test KeyInputStream with underlying ECBlockInputStream. + */ +public class TestKeyInputStreamEC { + + @Test Review comment: For this test, I wonder if it would be better if we avoiding manually adding streams into the KeyInputStream, and just pass the correct objects into `KeyInputStream.getFromOmKeyInfo` to make the problem appear. For example, I was able to create the test like this: ``` public class TestKeyInputStreamEC { @Test public void testReadAgainstLargeBlockGroup() throws IOException { int dataBlocks = 10; int parityBlocks = 4; ECReplicationConfig ec10And4RepConfig = new ECReplicationConfig(dataBlocks, parityBlocks, ECReplicationConfig.EcCodec.RS, (int)(1 * MB)); // default blockSize of 256MB with EC 10+4 makes a large block group long blockSize = 256 * MB; long blockLength = blockSize * dataBlocks; OmKeyInfo keyInfo = createOmKeyInfo(ec10And4RepConfig, dataBlocks + parityBlocks, blockLength); BlockExtendedInputStream inputStream = new ECStreamTestUtil.TestBlockInputStream(new BlockID(1, 1), blockLength, ByteBuffer.allocate(100)); BlockInputStreamFactory mockStreamFactory = mock(BlockInputStreamFactory.class); when(mockStreamFactory.create(any(), any(), any(), any(), anyBoolean(), any(), any())).thenReturn(inputStream); try (LengthInputStream is = KeyInputStream.getFromOmKeyInfo(keyInfo, null, true, null, mockStreamFactory)) { byte[] buf = new byte[100]; int readBytes = is.read(buf, 0, 100); } } private OmKeyInfo createOmKeyInfo(ReplicationConfig repConf, int nodeCount, long blockLength) { Map<DatanodeDetails, Integer> dnMap = new HashMap<>(); for (int i = 0; i < nodeCount; i++) { dnMap.put(MockDatanodeDetails.randomDatanodeDetails(), i + 1); } Pipeline pipeline = Pipeline.newBuilder() .setState(Pipeline.PipelineState.CLOSED) .setId(PipelineID.randomId()) .setNodes(new ArrayList<>(dnMap.keySet())) .setReplicaIndexes(dnMap) .setReplicationConfig(repConf) .build(); OmKeyLocationInfo blockInfo = new OmKeyLocationInfo.Builder() .setBlockID(new BlockID(1, 1)) .setLength(blockLength) .setOffset(0) .setPipeline(pipeline) .setPartNumber(0) .build(); List<OmKeyLocationInfo> locations = new ArrayList<>(); locations.add(blockInfo); return new OmKeyInfo.Builder() .setBucketName("bucket") .setVolumeName("volume") .setDataSize(blockLength) .setKeyName("someKey") .setReplicationConfig(repConf) .addOmKeyLocationInfoGroup(new OmKeyLocationInfoGroup(0, locations)) .build(); } } ``` I did need to make a couple of small changes to the test util to make this work: ``` --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/read/ECStreamTestUtil.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/read/ECStreamTestUtil.java @@ -276,7 +276,7 @@ public BlockExtendedInputStream create(ReplicationConfig repConfig, private int ecReplicaIndex = 0; private static final byte EOF = -1; - TestBlockInputStream(BlockID blockId, long blockLen, ByteBuffer data) { + public TestBlockInputStream(BlockID blockId, long blockLen, ByteBuffer data) { this(blockId, blockLen, data, 0); } @@ -325,7 +325,7 @@ public long getLength() { @Override public long getRemaining() { - return data.remaining(); + return getLength() - getPos(); } @Override @@ -342,7 +342,7 @@ public int read(ByteBuffer buf) throws IOException { if (getRemaining() == 0) { return EOF; } - int toRead = Math.min(buf.remaining(), (int)getRemaining()); + int toRead = (int)Math.min(buf.remaining(), getRemaining()); for (int i = 0; i < toRead; i++) { if (shouldError && data.position() >= shouldErrorPosition) { throwError(); ``` I think this test is better as it uses the normal interface to create the KeyInputStream, and we avoid any `@VisibleForTesting` on KeyInputStream. What do you think? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
