chihsuan commented on code in PR #11752:
URL: https://github.com/apache/ozone/pull/11752#discussion_r4430277719
##########
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/protocolPB/TestOzoneManagerProtocolClientSideTranslatorPB.java:
##########
@@ -60,6 +73,167 @@ class TestOzoneManagerProtocolClientSideTranslatorPB {
omTransport, "test-client-id");
private static final OmDeleteKeys DELETE_KEYS = new OmDeleteKeys("vol",
"bucket", Arrays.asList("key1", "key2"));
+ @ParameterizedTest
+ @EnumSource(value = Type.class, names = {"CreateKey", "CreateFile",
"AllocateBlock"})
+ void retriesMissingPipelineAllocationForEachClientOperation(Type operation)
throws Exception {
+ OzoneManagerProtocolClientSideTranslatorPB client = retryingClient();
+ when(omTransport.submitRequest(any(OMRequest.class)))
+ .thenReturn(errorResponse(operation, Status.KEY_ALLOCATION_ERROR),
successResponse(operation));
+
+ invokeBlockAllocation(client, operation);
+
+ ArgumentCaptor<OMRequest> requests =
ArgumentCaptor.forClass(OMRequest.class);
+ verify(omTransport, times(2)).submitRequest(requests.capture());
+
assertThat(requests.getAllValues().get(0)).isEqualTo(requests.getAllValues().get(1));
+ assertThat(requests.getValue().getCmdType()).isEqualTo(operation);
+ }
+
+ @Test
+ void stopsRetryingMissingPipelineAndPreservesLastError() throws Exception {
+ Type operation = Type.CreateKey;
+ when(omTransport.submitRequest(any(OMRequest.class)))
+ .thenReturn(errorResponse(operation, Status.KEY_ALLOCATION_ERROR));
+
+ ByteArrayOutputStream stderr = new ByteArrayOutputStream();
+ PrintStream originalErr = System.err;
+ OMException error;
+ try {
+ System.setErr(new PrintStream(stderr, false,
StandardCharsets.UTF_8.name()));
+ error = assertThrows(OMException.class, () ->
retryingClient().openKey(keyArgs()));
+ } finally {
+ System.setErr(originalErr);
Review Comment:
nit: Could we use `GenericTestUtils.SystemErrCapturer` here and in
`stillRetriesScmSafeMode`? It restores `System.err` on close, so the manual
try/finally could go away.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMAllocateBlockRequest.java:
##########
@@ -75,6 +77,51 @@
*/
public class TestOMAllocateBlockRequest extends OMKeyRequestTests {
+ @Test
+ public void testMissingPipelineMapsToRetryableError() throws Exception {
+ String message = "Waited pipeline disappeared";
+ when(scmBlockLocationProtocol.allocateBlock(anyLong(), anyInt(), any(),
+ anyString(), any(ExcludeList.class), anyString()))
+ .thenThrow(new SCMException(message,
SCMException.ResultCodes.PIPELINE_NOT_FOUND));
+
+ OMAllocateBlockRequest request =
getOmAllocateBlockRequest(createAllocateBlockRequest());
+ OMException error = assertThrows(OMException.class,
+ () -> request.allocateBlock(replicationConfig, new ExcludeList(),
scmBlockSize, false,
+ UserInfo.newBuilder().build(), ozoneManager));
+
+ assertEquals(OMException.ResultCodes.KEY_ALLOCATION_ERROR,
error.getResult());
+ assertEquals(message, error.getMessage());
+ }
+
+ @Test
+ public void testScmSafeModeMappingIsPreserved() throws Exception {
+ when(scmBlockLocationProtocol.allocateBlock(anyLong(), anyInt(), any(),
+ anyString(), any(ExcludeList.class), anyString()))
+ .thenThrow(new SCMException("SCM is in safe mode",
SCMException.ResultCodes.SAFE_MODE_EXCEPTION));
+
+ OMAllocateBlockRequest request =
getOmAllocateBlockRequest(createAllocateBlockRequest());
+ OMException error = assertThrows(OMException.class,
+ () -> request.allocateBlock(replicationConfig, new ExcludeList(),
scmBlockSize, false,
+ UserInfo.newBuilder().build(), ozoneManager));
+
+ assertEquals(OMException.ResultCodes.SCM_IN_SAFE_MODE, error.getResult());
+ }
+
+ @Test
+ public void testOtherScmAllocationFailureIsNotReclassified() throws
Exception {
+ SCMException cause = new SCMException("No suitable datanode",
+ SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE);
+ when(scmBlockLocationProtocol.allocateBlock(anyLong(), anyInt(), any(),
+ anyString(), any(ExcludeList.class), anyString())).thenThrow(cause);
+
+ OMAllocateBlockRequest request =
getOmAllocateBlockRequest(createAllocateBlockRequest());
+ SCMException error = assertThrows(SCMException.class,
+ () -> request.allocateBlock(replicationConfig, new ExcludeList(),
scmBlockSize, false,
+ UserInfo.newBuilder().build(), ozoneManager));
+
+ assertSame(cause, error);
Review Comment:
nit: These three tests share the same stub and `allocateBlock` call. Would
it be worth extracting a small helper that takes the `SCMException` and returns
what gets thrown?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ClientVersion.java:
##########
@@ -42,6 +42,9 @@ public enum ClientVersion implements ComponentVersion {
"This client version has support for Object Store and File " +
"System Optimized Bucket Layouts."),
+ KEY_ALLOCATION_PIPELINE_NOT_FOUND(4,
Review Comment:
Thanks for checking! I think this tradeoff is fine. It seems no OM has
emitted `KEY_ALLOCATION_ERROR` since 1.3.0, after HDDS-3231 removed the last
uses. So only much older servers could trigger extra retries.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableRatisContainerProvider.java:
##########
@@ -136,9 +143,12 @@ public ContainerInfo getContainer(final long size,
LOG.error(
"Unable to allocate a block for the size: {}, repConfig: {}",
size, repConfig);
- throw new IOException(
- "Unable to allocate a container to the block of size: " + size
- + ", replicationConfig: " + repConfig + ". " + failureReason);
+ String message = "Unable to allocate a container to the block of size: " +
size
+ + ", replicationConfig: " + repConfig + ". " + failureReason;
+ if (missingPipeline != null) {
Review Comment:
A follow-up for the opens-then-closes case sounds good to me. I also noticed
a similar case. If the pipeline created here disappears [while we wait for
it](https://github.com/apache/ozone/blob/874070374e79d91905c4111097362eab4f3300a7/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableRatisContainerProvider.java#L95-L97),
the client still gets `INTERNAL_ERROR`. Could we include it in the follow-up
too?
--
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]
