FrankChen021 commented on code in PR #19892:
URL: https://github.com/apache/druid/pull/19892#discussion_r3735953711
##########
cloud/aws-common/src/test/java/org/apache/druid/common/aws/AWSClientConfigTest.java:
##########
@@ -39,84 +66,144 @@ private static ObjectMapper
mapperWithRuntimeInfo(RuntimeInfo runtimeInfo)
}
@Test
- public void testDefaultCrossRegionAccessEnabled() throws Exception
+ public void testDefaultRetryModeIsStandard()
{
- AWSClientConfig config = MAPPER.readValue("{}", AWSClientConfig.class);
- Assertions.assertNull(config.isForceGlobalBucketAccessEnabled());
- Assertions.assertFalse(config.isCrossRegionAccessEnabled());
+ final AWSClientConfig config = new AWSClientConfig();
+
+ Assertions.assertEquals(AWSClientConfig.RetryMode.STANDARD,
config.getRetryMode());
+ Assertions.assertInstanceOf(StandardRetryStrategy.class,
config.getRetryStrategy());
}
- @Test
- public void testCrossRegionAccessEnabledExplicitlySet() throws Exception
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("retryModeStrategies")
+ public void testEachRetryModeBuildsItsStrategy(
+ AWSClientConfig.RetryMode mode,
+ Class<? extends RetryStrategy> expected
+ )
{
- AWSClientConfig config = MAPPER.readValue("{\"crossRegionAccessEnabled\":
true}", AWSClientConfig.class);
- Assertions.assertNull(config.isForceGlobalBucketAccessEnabled());
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
+ Assertions.assertInstanceOf(expected, mode.createStrategy());
}
+ /**
+ * Guards {@link #retryModeStrategies} against a mode being added without a
strategy expectation.
+ */
@Test
- public void testNewConfigTakesPrecedenceOverDeprecatedWhenBothSet() throws
Exception
+ public void testEveryRetryModeHasAStrategyExpectation()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"forceGlobalBucketAccessEnabled\": true,
\"crossRegionAccessEnabled\": false}",
- AWSClientConfig.class
+ Assertions.assertEquals(AWSClientConfig.RetryMode.values().length,
retryModeStrategies().count());
+ }
+
+ private static Stream<Arguments> retryModeStrategies()
+ {
+ return Stream.of(
+ Arguments.of(AWSClientConfig.RetryMode.STANDARD,
StandardRetryStrategy.class),
+ Arguments.of(AWSClientConfig.RetryMode.ADAPTIVE,
AdaptiveRetryStrategy.class),
+ Arguments.of(AWSClientConfig.RetryMode.LEGACY,
LegacyRetryStrategy.class)
);
- Assertions.assertFalse(config.isCrossRegionAccessEnabled());
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"adaptive", "ADAPTIVE", "Adaptive"})
+ public void testRetryModeParsingIsCaseInsensitive(String value)
+ {
+ Assertions.assertEquals(AWSClientConfig.RetryMode.ADAPTIVE,
AWSClientConfig.RetryMode.fromString(value));
}
@Test
- public void testNewConfigTrueWinsOverDeprecatedFalse() throws Exception
+ public void testRetryModeBindsFromItsProperty()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"forceGlobalBucketAccessEnabled\": false,
\"crossRegionAccessEnabled\": true}",
- AWSClientConfig.class
+ Assertions.assertEquals(
+ AWSClientConfig.RetryMode.ADAPTIVE,
+ bind(Map.of("retryMode", "adaptive")).getRetryMode()
);
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
}
@Test
- public void testDeprecatedForceGlobalBucketAccessAloneTrue() throws Exception
+ public void testRetryModeSerializesToItsPropertyValue()
+ {
+ Assertions.assertEquals("adaptive",
MAPPER.convertValue(AWSClientConfig.RetryMode.ADAPTIVE, String.class));
+ }
+
+ /**
+ * Binding the config is the last point at which a bad mode can be reported
against the property that set it, so it
+ * has to fail here rather than when some client is first built.
+ */
+ @Test
+ public void testUnrecognizedRetryModeIsRejectedWhenConfigIsBound()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"forceGlobalBucketAccessEnabled\": true}",
- AWSClientConfig.class
+ final IllegalArgumentException e = Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> bind(Map.of("retryMode", "aggressive"))
);
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
+
+ final Throwable rootCause = Throwables.getRootCause(e);
+ Assertions.assertInstanceOf(IAE.class, rootCause);
+ Assertions.assertTrue(rootCause.getMessage().contains("aggressive"));
}
@Test
- public void testDeprecatedNotSetFallsThroughToCrossRegion() throws Exception
+ public void testUnsetAttemptCountLeavesTheCountTheModeDefines()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"crossRegionAccessEnabled\": true}",
- AWSClientConfig.class
+ final AWSClientConfig config = new AWSClientConfig();
+
+ Assertions.assertNull(config.getMaxAttempts());
+ Assertions.assertEquals(
+ AWSClientConfig.RetryMode.STANDARD.createStrategy().maxAttempts(),
+ config.getRetryStrategy().maxAttempts()
);
- Assertions.assertNull(config.isForceGlobalBucketAccessEnabled());
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
}
@Test
- public void testDefaultMaxConnectionsKeepsAwsSdkFloorOnSmallHost() throws
Exception
+ public void testConfiguredAttemptCountIsApplied()
{
- AWSClientConfig config = mapperWithRuntimeInfo(new
FixedProcessorsRuntimeInfo(8))
- .readValue("{}", AWSClientConfig.class);
- Assertions.assertEquals(50, config.getMaxConnections());
+ Assertions.assertEquals(8, bind(Map.of("maxRetryAttempts",
8)).getRetryStrategy().maxAttempts());
Review Comment:
[P1] Use the renamed maxAttempts property in the binding test
AWSClientConfig now declares maxAttempts, but this test still binds
maxRetryAttempts. The plain ObjectMapper rejects that unknown property, so the
test throws before asserting the configured attempt count. Update the key to
maxAttempts or add an intentional compatibility alias.
--
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]
