Copilot commented on code in PR #10633:
URL: https://github.com/apache/ozone/pull/10633#discussion_r3559212186
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMAllocateBlockRequest.java:
##########
@@ -225,6 +252,184 @@ protected OMRequest doPreExecute(OMRequest
originalOMRequest)
return modifiedOmRequest;
}
+ @Test
+ public void testAllocateBlockSendsClientMachineToScmWhenFlagOff() throws
Exception {
+ // Flag off (default): OM must NOT sort; SCM receives the real client
address
+ // so it performs the sort.
+ KeyManager mockKeyManager = mock(KeyManager.class);
+ when(mockKeyManager.isSortDatanodesForWriteEnabled()).thenReturn(false);
+ when(ozoneManager.getKeyManager()).thenReturn(mockKeyManager);
+
+ OMAllocateBlockRequest request =
+
getOmAllocateBlockRequest(createAllocateBlockRequestWithSort("1.2.3.4"));
+ request.preExecute(ozoneManager);
+
+ ArgumentCaptor<String> clientMachine =
ArgumentCaptor.forClass(String.class);
+ verify(scmBlockLocationProtocol).allocateBlock(anyLong(), anyInt(), any(),
+ any(), any(), clientMachine.capture());
+ assertEquals("1.2.3.4", clientMachine.getValue());
+ verify(mockKeyManager, never()).sortDatanodesForWrite(any(), anyString());
+ }
+
+ @Test
+ public void testAllocateBlockDoesNotSendClientMachineToScm() throws
Exception {
+ // OM now sorts the write pipeline locally, so SCM must receive an empty
+ // clientMachine even when the client requests sorted datanodes.
+ KeyManager mockKeyManager = mock(KeyManager.class);
+ when(mockKeyManager.isSortDatanodesForWriteEnabled()).thenReturn(true);
+ when(mockKeyManager.sortDatanodesForWrite(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(0));
+ when(ozoneManager.getKeyManager()).thenReturn(mockKeyManager);
+
when(ozoneManager.getClusterMapAllowNull()).thenReturn(mock(NetworkTopology.class));
+
+ OMAllocateBlockRequest request =
+
getOmAllocateBlockRequest(createAllocateBlockRequestWithSort("1.2.3.4"));
+ request.preExecute(ozoneManager);
+
+ ArgumentCaptor<String> clientMachine =
ArgumentCaptor.forClass(String.class);
+ verify(scmBlockLocationProtocol).allocateBlock(anyLong(), anyInt(), any(),
+ any(), any(), clientMachine.capture());
+ assertEquals("", clientMachine.getValue());
+ }
+
+ @Test
+ public void testAllocateBlockFallsBackToScmWhenTopologyUnavailable() throws
Exception {
+ KeyManager mockKeyManager = mock(KeyManager.class);
+ when(mockKeyManager.isSortDatanodesForWriteEnabled()).thenReturn(true);
+ when(ozoneManager.getKeyManager()).thenReturn(mockKeyManager);
+
+ OMAllocateBlockRequest request =
+
getOmAllocateBlockRequest(createAllocateBlockRequestWithSort("1.2.3.4"));
+ request.preExecute(ozoneManager);
+
+ ArgumentCaptor<String> clientMachine =
ArgumentCaptor.forClass(String.class);
+ verify(scmBlockLocationProtocol).allocateBlock(anyLong(), anyInt(), any(),
+ any(), any(), clientMachine.capture());
+ assertEquals("1.2.3.4", clientMachine.getValue());
+ verify(mockKeyManager, never()).sortDatanodesForWrite(any(), anyString());
+ }
+
+ @Test
+ public void testAllocateBlockSortsSharedPipelineOnce() throws Exception {
+ // Two blocks on the same 3-node pipeline must be sorted once, and the
+ // sorted order must land in every block's pipeline.
+ List<DatanodeDetails> nodes = Arrays.asList(
+ MockDatanodeDetails.randomDatanodeDetails(),
+ MockDatanodeDetails.randomDatanodeDetails(),
+ MockDatanodeDetails.randomDatanodeDetails());
+ Pipeline pipeline = Pipeline.newBuilder()
+ .setState(Pipeline.PipelineState.OPEN)
+ .setId(PipelineID.randomId())
+ .setReplicationConfig(
+ StandaloneReplicationConfig.getInstance(ReplicationFactor.THREE))
+ .setNodes(nodes)
+ .build();
+ AllocatedBlock.Builder blockBuilder =
+ new AllocatedBlock.Builder().setPipeline(pipeline);
+ when(scmBlockLocationProtocol.allocateBlock(anyLong(), anyInt(), any(),
+ anyString(), any(ExcludeList.class), anyString())).thenAnswer(inv -> {
+ int num = inv.getArgument(1);
+ List<AllocatedBlock> blocks = new ArrayList<>(num);
+ for (int i = 0; i < num; i++) {
+ blockBuilder.setContainerBlockID(
+ new ContainerBlockID(CONTAINER_ID + i, LOCAL_ID + i));
+ blocks.add(blockBuilder.build());
+ }
+ return blocks;
+ });
+
+ List<DatanodeDetails> sortedOrder = new ArrayList<>(nodes);
+ Collections.reverse(sortedOrder);
+ KeyManager mockKeyManager = mock(KeyManager.class);
+ when(mockKeyManager.isSortDatanodesForWriteEnabled()).thenReturn(true);
+ when(mockKeyManager.sortDatanodesForWrite(any(), any()))
+ .thenAnswer(inv -> sortedOrder);
+ when(ozoneManager.getKeyManager()).thenReturn(mockKeyManager);
+
when(ozoneManager.getClusterMapAllowNull()).thenReturn(mock(NetworkTopology.class));
+
+ OMAllocateBlockRequest request =
+ getOmAllocateBlockRequest(createAllocateBlockRequest());
+ // requestedSize spans two scmBlockSize blocks on the same pipeline.
+ List<OmKeyLocationInfo> locations =
request.allocateBlock(replicationConfig,
+ new ExcludeList(), 2 * scmBlockSize, true,
+ UserInfo.newBuilder().setRemoteAddress("1.2.3.4").build(),
ozoneManager);
+
+ // Sorted once for the shared pipeline...
+ verify(mockKeyManager, times(1)).sortDatanodesForWrite(any(),
eq("1.2.3.4"));
+ // ...and the sorted order is applied to every block's pipeline.
+ assertEquals(2, locations.size());
+ for (OmKeyLocationInfo location : locations) {
+ assertEquals(sortedOrder, location.getPipeline().getNodesInOrder());
+ }
+ }
+
+ @Test
+ public void testAllocateBlockKeepsOrderWhenRemoteAddressEmpty() throws
Exception {
+ // Sort enabled and topology available, but the client has no remote
address:
+ // OM must not sort, SCM receives an empty clientMachine, and the pipeline
+ // order is preserved.
+ List<DatanodeDetails> nodes = Arrays.asList(
+ MockDatanodeDetails.randomDatanodeDetails(),
+ MockDatanodeDetails.randomDatanodeDetails(),
+ MockDatanodeDetails.randomDatanodeDetails());
+ Pipeline pipeline = Pipeline.newBuilder()
+ .setState(Pipeline.PipelineState.OPEN)
+ .setId(PipelineID.randomId())
+ .setReplicationConfig(
+ StandaloneReplicationConfig.getInstance(ReplicationFactor.THREE))
+ .setNodes(nodes)
+ .build();
+ AllocatedBlock block = new AllocatedBlock.Builder().setPipeline(pipeline)
+ .setContainerBlockID(new ContainerBlockID(CONTAINER_ID,
LOCAL_ID)).build();
+ ArgumentCaptor<String> clientMachine =
ArgumentCaptor.forClass(String.class);
+ when(scmBlockLocationProtocol.allocateBlock(anyLong(), anyInt(), any(),
+ anyString(), any(ExcludeList.class), clientMachine.capture()))
+ .thenReturn(Collections.singletonList(block));
+
+ KeyManager mockKeyManager = mock(KeyManager.class);
+ when(mockKeyManager.isSortDatanodesForWriteEnabled()).thenReturn(true);
+ when(ozoneManager.getKeyManager()).thenReturn(mockKeyManager);
+
when(ozoneManager.getClusterMapAllowNull()).thenReturn(mock(NetworkTopology.class));
+
+ OMAllocateBlockRequest request =
+ getOmAllocateBlockRequest(createAllocateBlockRequest());
+ List<OmKeyLocationInfo> locations =
request.allocateBlock(replicationConfig,
+ new ExcludeList(), scmBlockSize, true,
+ UserInfo.newBuilder().setRemoteAddress("").build(), ozoneManager);
+
+ assertEquals("", clientMachine.getValue());
+ verify(mockKeyManager, never()).sortDatanodesForWrite(any(), anyString());
+ assertEquals(1, locations.size());
+ assertEquals(nodes, locations.get(0).getPipeline().getNodes());
+ }
Review Comment:
This assertion is checking `getNodes()` (the pipeline’s static node list),
but the write-sort logic updates `nodesInOrder` via
`copyWithNodesInOrder(...)`. If a regression caused write sorting to run (or
SCM returned `nodesInOrder` reordered), this test would still pass. Assert
against `getNodesInOrder()` to validate the actual write order.
--
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]
