HDDS-14717. Improve test coverage for OzoneManagerStateMachine#9825
HDDS-14717. Improve test coverage for OzoneManagerStateMachine#9825fapifta wants to merge 1 commit intoapache:masterfrom
Conversation
Generated-by: Claude - Opus 4.6
| public void initialize(RaftServer server, RaftGroupId id, RaftStorage raftStorage) throws IOException { | ||
| getLifeCycle().startAndTransition(() -> { | ||
| super.initialize(server, id, raftStorage); | ||
| storage.init(raftStorage); |
There was a problem hiding this comment.
why init() is removed here? its functional change, not just adding new tests
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@szetszwo Is storage.init(raftStorage) needed here?
There was a problem hiding this comment.
storage is a unused field (except here). We definite could (and should) remove it.
| assertEquals(3, peerIds.size()); | ||
| assertTrue(peerIds.contains("om1")); | ||
| assertTrue(peerIds.contains("om2")); | ||
| assertTrue(peerIds.contains("om3")); |
There was a problem hiding this comment.
If any of these assertions fail, we don't get much info about the actual state (which peers were present). This one is better:
assertThat(peerIds)
.containsExactlyInAnyOrder("om1", "om2", "om3");
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good for OzoneManagerStateMachine
(Sorry that I have not checked the changes in the tests.)
| private final SimpleStateMachineStorage storage = | ||
| new SimpleStateMachineStorage(); |
There was a problem hiding this comment.
👍 Good catch for removing this unused field.
| public void initialize(RaftServer server, RaftGroupId id, RaftStorage raftStorage) throws IOException { | ||
| getLifeCycle().startAndTransition(() -> { | ||
| super.initialize(server, id, raftStorage); | ||
| storage.init(raftStorage); |
There was a problem hiding this comment.
storage is a unused field (except here). We definite could (and should) remove it.
4 participants
What changes were proposed in this pull request?
Provide a one-liner summary of the changes in the PR Title field above.
It should be in the form of
HDDS-1234. Short summary of the change.Please describe your PR in detail:
This PR and its description was fully done by Claude-code based on my direction and requests.
tests covering startTransaction, preAppendTransaction, applyTransaction, runCommand, processResponse,
createErrorResponse, query, takeSnapshot, loadSnapshotInfoFromDB, leader/configuration change
notifications, snapshot installation, and lifecycle methods (pause/stop/close).
OzoneManagerDoubleBuffer, RequestHandler, ExecutorService, NettyMetrics), replacing the heavy setup that
required a real OmMetadataManagerImpl and Ratis server.
package-private for direct unit testing.
transaction index and command type when handleWriteRequestImpl encounters an exception.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14717
How was this patch tested?
New tests were running locally.
Generated-by: Claude - Opus 4.6