Skip to content

HDDS-14717. Improve test coverage for OzoneManagerStateMachine #9825

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 1 commit into from
Feb 28, 2026
Merged

HDDS-14717. Improve test coverage for OzoneManagerStateMachine #9825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 26 additions & 9 deletions ...zone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@
import org.apache.ratis.statemachine.SnapshotInfo;
import org.apache.ratis.statemachine.TransactionContext;
import org.apache.ratis.statemachine.impl.BaseStateMachine;
import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
import org.apache.ratis.thirdparty.com.google.protobuf.ByteString;
import org.apache.ratis.util.ExitUtils;
import org.apache.ratis.util.IOUtils;
Expand All @@ -94,8 +93,6 @@ public class OzoneManagerStateMachine extends BaseStateMachine {
private static final String AUDIT_PARAM_PREVIOUS_LEADER = "previousLeader";
private static final String AUDIT_PARAM_NEW_LEADER = "newLeader";
private RaftPeerId previousLeaderId = null;
private final SimpleStateMachineStorage storage =
new SimpleStateMachineStorage();
Comment on lines -97 to -98
Copy link
Contributor

@szetszwo szetszwo Feb 27, 2026

Choose a reason for hiding this comment

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

👍 Good catch for removing this unused field.

private final OzoneManager ozoneManager;
private RequestHandler handler;
private volatile OzoneManagerDoubleBuffer ozoneManagerDoubleBuffer;
Expand Down Expand Up @@ -135,15 +132,32 @@ public OzoneManagerStateMachine(OzoneManagerRatisServer ratisServer,
this.nettyMetrics = NettyMetrics.create();
}

@VisibleForTesting
OzoneManagerStateMachine(OzoneManager ozoneManager,
OzoneManagerDoubleBuffer doubleBuffer,
RequestHandler handler,
ExecutorService executorService,
NettyMetrics nettyMetrics) {
this.isTracingEnabled = false;
this.ozoneManager = ozoneManager;
this.threadPrefix = "";
this.ozoneManagerDoubleBuffer = doubleBuffer;
this.handler = handler;
this.executorService = executorService;
ThreadFactory installSnapshotThreadFactory = new ThreadFactoryBuilder()
.setNameFormat("TestInstallSnapshotThread").build();
this.installSnapshotExecutor =
HadoopExecutors.newSingleThreadExecutor(installSnapshotThreadFactory);
this.nettyMetrics = nettyMetrics;
}

/**
* Initializes the State Machine with the given server, group and storage.
*/
@Override
public void initialize(RaftServer server, RaftGroupId id,
RaftStorage raftStorage) throws IOException {
public void initialize(RaftServer server, RaftGroupId id, RaftStorage raftStorage) throws IOException {
getLifeCycle().startAndTransition(() -> {
super.initialize(server, id, raftStorage);
storage.init(raftStorage);
Copy link

@yandrey321 yandrey321 Feb 25, 2026

Choose a reason for hiding this comment

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

why init() is removed here? its functional change, not just adding new tests

Copy link
Contributor Author

@fapifta fapifta Feb 26, 2026

Choose a reason for hiding this comment

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

If you look through the earlier code, this storage variable was created and then never used outside of this call in the OM state machine.
If you check the BaseStateMachine, that also does not do anything with the RaftStorage given to super.initialize as a parameter.
In Ozone Manager, the state machine's storage related methods are mapped to a state stored in RocksDB.
See BaseStateMachine.getStateMachineStorage() where implementation is empty, and does not return anything, which means storage is not used there, as it is not used in the initialize method either. This is not overridden by OzoneManagerStateMachine.
See the usage of getStateMachineStorage() method, it is used only from the getLatestSnapshot() method in the BaseStateMachineStorage class, this method is overridden by OzoneManagerStateMachine, where getLatestSnapshot() uses a call to RocksDB to get the SnapshotInfo.

Based on all of this, I don't see a real use of the variable, and this seems to me dead code, please let me know if you disagree and/or if I am missing something.

Copy link
Contributor

@adoroszlai adoroszlai Feb 27, 2026

Choose a reason for hiding this comment

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

@szetszwo Is storage.init(raftStorage) needed here?

Copy link
Contributor

@szetszwo szetszwo Feb 27, 2026

Choose a reason for hiding this comment

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

storage is a unused field (except here). We definite could (and should) remove it.

LOG.info("{}: initialize {} with {}", getId(), id, getLastAppliedTermIndex());
});
}
Expand Down Expand Up @@ -419,7 +433,8 @@ public CompletableFuture<Message> applyTransaction(TransactionContext trx) {
}
}

private Message processResponse(OMResponse omResponse) {
@VisibleForTesting
Message processResponse(OMResponse omResponse) {
if (!omResponse.getSuccess()) {
// INTERNAL_ERROR or METADATA_ERROR are considered as critical errors.
// In such cases, OM must be terminated instead of completing the future exceptionally,
Expand Down Expand Up @@ -593,7 +608,8 @@ public void close() {
* @param request OMRequest
* @return response from OM
*/
private OMResponse runCommand(OMRequest request, TermIndex termIndex) {
@VisibleForTesting
OMResponse runCommand(OMRequest request, TermIndex termIndex) {
try {
ExecutionContext context = ExecutionContext.of(termIndex.getIndex(), termIndex);
final OMClientResponse omClientResponse = handler.handleWriteRequest(
Expand All @@ -617,7 +633,8 @@ private OMResponse runCommand(OMRequest request, TermIndex termIndex) {
return null;
}

private OMResponse createErrorResponse(
@VisibleForTesting
OMResponse createErrorResponse(
OMRequest omRequest, IOException exception, TermIndex termIndex) {
OMResponse.Builder omResponseBuilder = OMResponse.newBuilder()
.setStatus(OzoneManagerRatisUtils.exceptionToResponseStatus(exception))
Expand Down
Loading