Skip to content

HDDS-12604. Reduce duplication in TestContainerStateMachine #8104

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

Merged
adoroszlai merged 11 commits into from
Mar 25, 2025
Merged

HDDS-12604. Reduce duplication in TestContainerStateMachine #8104

Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
06c1508
HDDS-12604. Reduce duplication in TestContainerStateMachine
chiacyu Mar 17, 2025
cca89ed
substitute with function
chiacyu Mar 17, 2025
f74bd2b
Refactor functions
chiacyu Mar 17, 2025
4bc69ef
Fixed styles
chiacyu Mar 17, 2025
07394a5
HDDS-12604. Reduce duplication in TestContainerStateMachine
chiacyu Mar 17, 2025
5597285
Merge from master
chiacyu Mar 17, 2025
44ff1b3
refactor functions
chiacyu Mar 18, 2025
53e8207
Apply comments
chiacyu Mar 20, 2025
6bb5098
Fixed styles
chiacyu Mar 20, 2025
804c5c1
Fixed findbugs
chiacyu Mar 20, 2025
20b6e92
Applied comments
chiacyu Mar 22, 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
114 changes: 54 additions & 60 deletions ...pache/hadoop/ozone/container/common/transport/server/ratis/TestContainerStateMachine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,31 @@ public void testWriteFailure(boolean failWithException) throws ExecutionExceptio
TransactionContext trx = mock(TransactionContext.class);
ContainerStateMachine.Context context = mock(ContainerStateMachine.Context.class);
when(trx.getStateMachineContext()).thenReturn(context);

setUpMockDispatcherReturn(failWithException);
setUpMockRequestProtoReturn(context, "Test Data", 1, 1);

@peterxcli peterxcli Mar 19, 2025

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.

ditto

Suggested change
setUpMockRequestProtoReturn(context, "Test Data", 1, 1);
setUpMockRequestProtoReturn(context, 1, 1);


AtomicReference<Throwable> throwable = new AtomicReference<>(null);
Function<Throwable, ? extends Message> throwableSetter = getThrowableSetter(throwable);

stateMachine.write(entry, trx).exceptionally(throwableSetter).get();
verify(dispatcher, times(1)).dispatch(any(ContainerProtos.ContainerCommandRequestProto.class),
any(DispatcherContext.class));
reset(dispatcher);
assertNotNull(throwable.get());
assertResults(failWithException, throwable);

// Writing data to another container(containerId 2) should also fail.
setUpMockRequestProtoReturn(context, "Test Data", 2, 1);

@peterxcli peterxcli Mar 19, 2025

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.

ditto

Suggested change
setUpMockRequestProtoReturn(context, "Test Data", 2, 1);
setUpMockRequestProtoReturn(context, 2, 1);

stateMachine.write(entryNext, trx).exceptionally(throwableSetter).get();
verify(dispatcher, times(0)).dispatch(any(ContainerProtos.ContainerCommandRequestProto.class),
any(DispatcherContext.class));
assertInstanceOf(StorageContainerException.class, throwable.get());
StorageContainerException sce = (StorageContainerException) throwable.get();
assertEquals(ContainerProtos.Result.CONTAINER_UNHEALTHY, sce.getResult());
}

public final void setUpMockDispatcherReturn(boolean failWithException) {

@peterxcli peterxcli Mar 19, 2025

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.

nit:

Suggested change
public final void setUpMockDispatcherReturn(boolean failWithException) {
private final void setUpMockDispatcherReturn(boolean failWithException) {

if (failWithException) {
when(dispatcher.dispatch(any(), any())).thenThrow(new RuntimeException());
} else {
Expand All @@ -137,45 +162,46 @@ public void testWriteFailure(boolean failWithException) throws ExecutionExceptio
.setResult(ContainerProtos.Result.CONTAINER_INTERNAL_ERROR)
.build());
}
}

public final void setUpMockRequestProtoReturn(ContainerStateMachine.Context context, String content,

@peterxcli peterxcli Mar 19, 2025

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.

Suggested change
public final void setUpMockRequestProtoReturn(ContainerStateMachine.Context context, String content,
private final void setUpMockRequestProtoReturn(ContainerStateMachine.Context context,

Since all content are the same, it can be moved to class Initialization

abstract class TestContainerStateMachine {
  ...
  private final ByteString CONTAINER_DATA = ByteString.copyFromUtf8("Test Data");

int containerId, int localId) {
when(context.getRequestProto()).thenReturn(ContainerProtos.ContainerCommandRequestProto.newBuilder()
.setCmdType(ContainerProtos.Type.WriteChunk).setWriteChunk(
ContainerProtos.WriteChunkRequestProto.newBuilder().setData(ByteString.copyFromUtf8("Test Data"))
ContainerProtos.WriteChunkRequestProto.newBuilder().setData(ByteString.copyFromUtf8(content))
.setBlockID(
ContainerProtos.DatanodeBlockID.newBuilder().setContainerID(1).setLocalID(1).build()).build())
.setContainerID(1)
ContainerProtos.DatanodeBlockID.newBuilder().setContainerID(containerId)
.setLocalID(localId).build()).build())
.setContainerID(containerId)
.setDatanodeUuid(UUID.randomUUID().toString()).build());
AtomicReference<Throwable> throwable = new AtomicReference<>(null);
}

public final Function<Throwable, ? extends Message> getThrowableSetter(AtomicReference<Throwable> throwable) {

@peterxcli peterxcli Mar 19, 2025

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.

another suggestion is replace getThrowableSetter with:

private static class ThrowableCatcher {
  private final AtomicReference<Throwable> caught = new AtomicReference<>(null);
  
  public Function<Throwable, ? extends Message> asSetter() {
    return t -> {
      caught.set(t);
      return null;
    };
  }

  public Throwable getReceived() {
    return caught.get();
  }

  public void reset() {
    caught.set(null);
  }
}

Then we can use:

ThrowableCatcher catcher = new ThrowableCatcher();
stateMachine.write(entry, trx).exceptionally(catcher.asSetter()).get();
assertNotNull(catcher.getReceived());

But this is up to you, this change has few benefit at this time.

Function<Throwable, ? extends Message> throwableSetter = t -> {
throwable.set(t);
return null;
};
stateMachine.write(entry, trx).exceptionally(throwableSetter).get();
verify(dispatcher, times(1)).dispatch(any(ContainerProtos.ContainerCommandRequestProto.class),
any(DispatcherContext.class));
reset(dispatcher);
assertNotNull(throwable.get());
return throwableSetter;
}

public final void assertResults(boolean failWithException, AtomicReference<Throwable> throwable) {
if (failWithException) {
assertInstanceOf(RuntimeException.class, throwable.get());
} else {
assertInstanceOf(StorageContainerException.class, throwable.get());
StorageContainerException sce = (StorageContainerException) throwable.get();
assertEquals(ContainerProtos.Result.CONTAINER_INTERNAL_ERROR, sce.getResult());
}
// Writing data to another container(containerId 2) should also fail.
when(context.getRequestProto()).thenReturn(ContainerProtos.ContainerCommandRequestProto.newBuilder()
}

public final void setUpLogProtoReturn(ContainerStateMachine.Context context, int containerId, int localId) {
when(context.getLogProto()).thenReturn(ContainerProtos.ContainerCommandRequestProto.newBuilder()
.setCmdType(ContainerProtos.Type.WriteChunk).setWriteChunk(
ContainerProtos.WriteChunkRequestProto.newBuilder().setData(ByteString.copyFromUtf8("Test Data"))
.setBlockID(
ContainerProtos.DatanodeBlockID.newBuilder().setContainerID(2).setLocalID(1).build()).build())
.setContainerID(2)
ContainerProtos.WriteChunkRequestProto.newBuilder().setBlockID(
ContainerProtos.DatanodeBlockID.newBuilder().
setContainerID(containerId).setLocalID(localId).build()).build())
.setContainerID(containerId)
.setDatanodeUuid(UUID.randomUUID().toString()).build());
stateMachine.write(entryNext, trx).exceptionally(throwableSetter).get();
verify(dispatcher, times(0)).dispatch(any(ContainerProtos.ContainerCommandRequestProto.class),
any(DispatcherContext.class));
assertInstanceOf(StorageContainerException.class, throwable.get());
StorageContainerException sce = (StorageContainerException) throwable.get();
assertEquals(ContainerProtos.Result.CONTAINER_UNHEALTHY, sce.getResult());
}

@ParameterizedTest
Expand All @@ -189,40 +215,20 @@ public void testApplyTransactionFailure(boolean failWithException) throws Execut
ContainerStateMachine.Context context = mock(ContainerStateMachine.Context.class);
when(trx.getLogEntry()).thenReturn(entry);
when(trx.getStateMachineContext()).thenReturn(context);
if (failWithException) {
when(dispatcher.dispatch(any(), any())).thenThrow(new RuntimeException());
} else {
when(dispatcher.dispatch(any(), any())).thenReturn(ContainerProtos.ContainerCommandResponseProto
.newBuilder().setCmdType(ContainerProtos.Type.WriteChunk)
.setResult(ContainerProtos.Result.CONTAINER_INTERNAL_ERROR)
.build());
}

setUpMockDispatcherReturn(failWithException);
// Failing apply transaction on congtainer 1.
when(context.getLogProto()).thenReturn(ContainerProtos.ContainerCommandRequestProto.newBuilder()
.setCmdType(ContainerProtos.Type.WriteChunk).setWriteChunk(
ContainerProtos.WriteChunkRequestProto.newBuilder().setBlockID(
ContainerProtos.DatanodeBlockID.newBuilder().setContainerID(1).setLocalID(1).build()).build())
.setContainerID(1)
.setDatanodeUuid(UUID.randomUUID().toString()).build());
setUpLogProtoReturn(context, 1, 1);
AtomicReference<Throwable> throwable = new AtomicReference<>(null);
Function<Throwable, ? extends Message> throwableSetter = t -> {
throwable.set(t);
return null;
};
Function<Throwable, ? extends Message> throwableSetter = getThrowableSetter(throwable);
//apply transaction will fail because of runtime exception thrown by dispatcher, which marks the first
// failure on container 1.
stateMachine.applyTransaction(trx).exceptionally(throwableSetter).get();
verify(dispatcher, times(1)).dispatch(any(ContainerProtos.ContainerCommandRequestProto.class),
any(DispatcherContext.class));
reset(dispatcher);
assertNotNull(throwable.get());
if (failWithException) {
assertInstanceOf(RuntimeException.class, throwable.get());
} else {
assertInstanceOf(StorageContainerException.class, throwable.get());
StorageContainerException sce = (StorageContainerException) throwable.get();
assertEquals(ContainerProtos.Result.CONTAINER_INTERNAL_ERROR, sce.getResult());
}
assertResults(failWithException, throwable);
// Another apply transaction on same container 1 should fail because the previous apply transaction failed.
stateMachine.applyTransaction(trx).exceptionally(throwableSetter).get();
verify(dispatcher, times(0)).dispatch(any(ContainerProtos.ContainerCommandRequestProto.class),
Expand All @@ -233,13 +239,7 @@ public void testApplyTransactionFailure(boolean failWithException) throws Execut

// Another apply transaction on a different container 2 shouldn't fail because the previous apply transaction
// failure was only on container 1.
when(context.getLogProto()).thenReturn(ContainerProtos.ContainerCommandRequestProto.newBuilder()
.setCmdType(ContainerProtos.Type.WriteChunk).setWriteChunk(
ContainerProtos.WriteChunkRequestProto.newBuilder().setBlockID(
ContainerProtos.DatanodeBlockID.newBuilder().setContainerID(2).setLocalID(1).build()).build())
.setContainerID(2)
.setDatanodeUuid(UUID.randomUUID().toString()).build());

setUpLogProtoReturn(context, 2, 1);
reset(dispatcher);
throwable.set(null);
when(dispatcher.dispatch(any(), any())).thenReturn(ContainerProtos.ContainerCommandResponseProto
Expand Down Expand Up @@ -275,13 +275,7 @@ public void testWriteTimout() throws Exception {
return null;
}).when(dispatcher).dispatch(any(), any());

when(context.getRequestProto()).thenReturn(ContainerProtos.ContainerCommandRequestProto.newBuilder()
.setCmdType(ContainerProtos.Type.WriteChunk).setWriteChunk(
ContainerProtos.WriteChunkRequestProto.newBuilder().setData(ByteString.copyFromUtf8("Test Data"))
.setBlockID(
ContainerProtos.DatanodeBlockID.newBuilder().setContainerID(1).setLocalID(1).build()).build())
.setContainerID(1)
.setDatanodeUuid(UUID.randomUUID().toString()).build());
setUpMockRequestProtoReturn(context, "Test data", 1, 1);

@peterxcli peterxcli Mar 19, 2025

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.

ditto

Suggested change
setUpMockRequestProtoReturn(context, "Test data", 1, 1);
setUpMockRequestProtoReturn(context, 1, 1);

AtomicReference<Throwable> throwable = new AtomicReference<>(null);
Function<Throwable, ? extends Message> throwableSetter = t -> {
throwable.set(t);
Expand Down