Skip to content

HDDS-12505. Use Mockito doReturn() in hadoop-hdds/client #8034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
chiacyu wants to merge 7 commits into from
Closed

HDDS-12505. Use Mockito doReturn() in hadoop-hdds/client #8034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4f49afd
HDDS-12505. Use Mockito doReturn() in hadoop-hdds/client
chiacyu Mar 8, 2025
4a23d3a
Fixed findingBugs
chiacyu Mar 8, 2025
4d837b0
Fixed syntax
chiacyu Mar 8, 2025
593f389
Fixed findingbugs
chiacyu Mar 9, 2025
fae4b19
fixed findingBugs
chiacyu Mar 9, 2025
9083eab
fixed findingBugs
chiacyu Mar 9, 2025
7d1f370
Fixed findingBugs
chiacyu Mar 9, 2025
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
12 changes: 12 additions & 0 deletions hadoop-hdds/client/dev-support/findbugsExcludeFile.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,16 @@
<Class name="org.apache.hadoop.hdds.scm.storage.ByteArrayReader"></Class>
<Bug pattern="EI_EXPOSE_REP2" /> <!-- "Deep copy byte[] has bad impact on performance" -->
</Match>
<Match>
<Class name="org.apache.hadoop.hdds.scm.storage.DummyBlockInputStreamWithRetry"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT" />
</Match>
<Match>
<Class name="org.apache.hadoop.hdds.scm.storage.TestBlockInputStream"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT" />
</Match>
<Match>
<Class name="org.apache.hadoop.ozone.client.io.TestBlockInputStreamFactoryImpl"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT" />
</Match>
Comment on lines +22 to +33
Copy link
Contributor

@adoroszlai adoroszlai Mar 9, 2025

Choose a reason for hiding this comment

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

Please do not add these.

  • It is completely non-scalable to add an exclusion for each test class.
  • We should reduce the number of exclusions, not increase it (HDDS-12342).
  • Only 3 classes are added here, but 5 classes are changed similarly. I'm guessing the others were not reported by spotbugs. The problem is that spotbugs analysis is not 100% accurate, and sometimes starts reporting existing problems only later, when making changes in other classes.

This is a spotbugs problem. @peterxcli reported it few weeks ago: #7963 (comment)

I don't know if the problem is fixed by more recent versions of spotbugs. Unfortunately we have to use an older version of spotbugs due to lots of new issues reported by newer one (HDDS-10150).

I think we should either

  • defer mass conversion to doReturn until we have a version of spotbugs where this is fixed, OR
  • add exclusion for this issue in all test classes using pattern matching, AND enable similar rule in PMD (subtask of HDDS-12338).

Copy link
Contributor Author

@chiacyu chiacyu Mar 9, 2025

Choose a reason for hiding this comment

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

Hi, @adoroszlai
Thanks for the point out, which one would be better? How about we hold on the doReturn() changes, but I would still create a subtask of HDDS-12338 for that? thanks!

Copy link
Contributor

@adoroszlai adoroszlai Mar 9, 2025

Choose a reason for hiding this comment

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

On quick glance I couldn't find similar rule in PMD.

</FindBugsFilter>
4 changes: 2 additions & 2 deletions ...ient/src/test/java/org/apache/hadoop/hdds/scm/storage/DummyBlockInputStreamWithRetry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
package org.apache.hadoop.hdds.scm.storage;

import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.Result.CONTAINER_NOT_FOUND;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.io.IOException;
import java.util.List;
Expand Down Expand Up @@ -63,7 +63,7 @@ final class DummyBlockInputStreamWithRetry
try {
BlockLocationInfo blockLocationInfo = mock(BlockLocationInfo.class);
Pipeline mockPipeline = MockPipeline.createPipeline(1);
when(blockLocationInfo.getPipeline()).thenReturn(mockPipeline);
doReturn(mockPipeline).when(blockLocationInfo).getPipeline();
return blockLocationInfo;
} catch (IOException e) {
throw new RuntimeException(e);
Expand Down
18 changes: 8 additions & 10 deletions ...op-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyInt;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.reset;
Expand Down Expand Up @@ -296,18 +297,18 @@ void refreshesPipelineOnReadFailure(IOException ex) throws Exception {
// GIVEN
Pipeline pipeline = MockPipeline.createSingleNodePipeline();
BlockLocationInfo blockLocationInfo = mock(BlockLocationInfo.class);
when(blockLocationInfo.getPipeline()).thenReturn(pipeline);
Pipeline newPipeline = MockPipeline.createSingleNodePipeline();
BlockLocationInfo newBlockLocationInfo = mock(BlockLocationInfo.class);

doReturn(pipeline).when(blockLocationInfo).getPipeline();
testRefreshesPipelineOnReadFailure(ex, blockLocationInfo,
id -> newBlockLocationInfo);

when(newBlockLocationInfo.getPipeline()).thenReturn(newPipeline);
doReturn(newPipeline).when(newBlockLocationInfo).getPipeline();
testRefreshesPipelineOnReadFailure(ex, blockLocationInfo,
id -> blockLocationInfo);

when(newBlockLocationInfo.getPipeline()).thenReturn(null);
doReturn(null).when(newBlockLocationInfo).getPipeline();
testRefreshesPipelineOnReadFailure(ex, blockLocationInfo,
id -> newBlockLocationInfo);
}
Expand Down Expand Up @@ -358,8 +359,7 @@ private static ChunkInputStream throwingChunkInputStream(IOException ex,
if (succeedOnRetry) {
stubbing.thenReturn(len);
}
when(stream.getRemaining())
.thenReturn((long) len);
doReturn((long) len).when(stream).getRemaining();
return stream;
}

Expand Down Expand Up @@ -415,15 +415,13 @@ public void testRefreshOnReadFailureAfterUnbuffer(IOException ex)
XceiverClientFactory clientFactory = mock(XceiverClientFactory.class);
XceiverClientSpi client = mock(XceiverClientSpi.class);
BlockLocationInfo blockLocationInfo = mock(BlockLocationInfo.class);
when(clientFactory.acquireClientForReadData(pipeline))
.thenReturn(client);
doReturn(client).when(clientFactory).acquireClientForReadData(pipeline);

final int len = 200;
final ChunkInputStream stream = throwingChunkInputStream(ex, len, true);

when(refreshFunction.apply(blockID))
.thenReturn(blockLocationInfo);
when(blockLocationInfo.getPipeline()).thenReturn(newPipeline);
doReturn(blockLocationInfo).when(refreshFunction).apply(blockID);
doReturn(newPipeline).when(blockLocationInfo).getPipeline();

OzoneClientConfig clientConfig = conf.getObject(OzoneClientConfig.class);
clientConfig.setChecksumVerify(false);
Expand Down
8 changes: 3 additions & 5 deletions ...nt/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockOutputStreamCorrectness.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
import static java.util.concurrent.Executors.newFixedThreadPool;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -157,8 +157,7 @@ private BlockOutputStream createBlockOutputStream(BufferPool bufferPool)
final Pipeline pipeline = MockPipeline.createRatisPipeline();

final XceiverClientManager xcm = mock(XceiverClientManager.class);
when(xcm.acquireClient(any()))
.thenReturn(new MockXceiverClientSpi(pipeline));
doReturn(new MockXceiverClientSpi(pipeline)).when(xcm).acquireClient(any());

OzoneClientConfig config = new OzoneClientConfig();
config.setStreamBufferSize(4 * 1024 * 1024);
Expand Down Expand Up @@ -186,8 +185,7 @@ private BlockOutputStream createBlockOutputStream(BufferPool bufferPool)
private ECBlockOutputStream createECBlockOutputStream(OzoneClientConfig clientConfig,
ECReplicationConfig repConfig, BlockID blockID, Pipeline pipeline) throws IOException {
final XceiverClientManager xcm = mock(XceiverClientManager.class);
when(xcm.acquireClient(any()))
.thenReturn(new MockXceiverClientSpi(pipeline));
doReturn(new MockXceiverClientSpi(pipeline)).when(xcm).acquireClient(any());

ContainerClientMetrics clientMetrics = ContainerClientMetrics.acquire();
StreamBufferArgs streamBufferArgs =
Expand Down
10 changes: 4 additions & 6 deletions ...op-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestChunkInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -244,19 +245,16 @@ public void connectsToNewPipeline() throws Exception {
Pipeline newPipeline = MockPipeline.createSingleNodePipeline();

Token<?> token = mock(Token.class);
when(token.encodeToUrlString())
.thenReturn("oldToken");
doReturn("oldToken").when(token).encodeToUrlString();
Token<?> newToken = mock(Token.class);
when(newToken.encodeToUrlString())
.thenReturn("newToken");
doReturn("newToken").when(newToken).encodeToUrlString();

AtomicReference<Pipeline> pipelineRef = new AtomicReference<>(pipeline);
AtomicReference<Token<?>> tokenRef = new AtomicReference<>(token);

XceiverClientFactory clientFactory = mock(XceiverClientFactory.class);
XceiverClientSpi client = mock(XceiverClientSpi.class);
when(clientFactory.acquireClientForReadData(any()))
.thenReturn(client);
doReturn(client).when(clientFactory).acquireClientForReadData(any());
ArgumentCaptor<ContainerCommandRequestProto> requestCaptor =
ArgumentCaptor.forClass(ContainerCommandRequestProto.class);
when(client.getPipeline())
Expand Down
3 changes: 2 additions & 1 deletion ...ient/src/test/java/org/apache/hadoop/ozone/client/io/TestBlockInputStreamFactoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;

import java.io.IOException;
import java.util.ArrayList;
Expand Down Expand Up @@ -59,7 +60,7 @@ public void testNonECGivesBlockInputStream() throws IOException {
1024 * 1024 * 10);
Pipeline pipeline = Mockito.spy(blockInfo.getPipeline());
blockInfo.setPipeline(pipeline);
Mockito.when(pipeline.getReplicaIndex(any(DatanodeDetails.class))).thenReturn(1);
doReturn(1).when(pipeline).getReplicaIndex(any(DatanodeDetails.class));
OzoneClientConfig clientConfig = conf.getObject(OzoneClientConfig.class);
clientConfig.setChecksumVerify(true);
BlockExtendedInputStream stream =
Expand Down