adoroszlai commented on code in PR #11096:
URL: https://github.com/apache/ozone/pull/11096#discussion_r4047536361
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/ratis/TestOzoneManagerRatisServer.java:
##########
@@ -275,4 +281,45 @@ public void
verifyRaftGroupIdGenerationWithCustomOmServiceId(@TempDir Path metaD
assertEquals(raftGroupId.toByteString().size(), 16);
newOmRatisServer.stop();
}
+
+ @Test
+ public void testRetryCacheExpiryTime(@TempDir Path ratisDir) {
+ assertEquals(300_000, retryCacheExpiryMillis(new OzoneConfiguration(),
ratisDir));
+
+ OzoneConfiguration currentKeyConf = new OzoneConfiguration();
+ currentKeyConf.set(CURRENT_RETRY_CACHE_KEY, "42s");
+ assertEquals(42_000, retryCacheExpiryMillis(currentKeyConf, ratisDir));
+
+ // The deprecated key must reach Ratis instead of being silently
overwritten, and must warn.
+ LogCapturer logCapturer =
LogCapturer.captureLogs(OzoneManagerRatisServer.class);
+ OzoneConfiguration deprecatedKeyConf = new OzoneConfiguration();
+ deprecatedKeyConf.set(DEPRECATED_RETRY_CACHE_KEY, "17s");
+ assertEquals(17_000, retryCacheExpiryMillis(deprecatedKeyConf, ratisDir));
+ assertThat(logCapturer.getOutput()).contains(deprecationWarning(17_000));
+
+ // A value without a unit suffix keeps the milliseconds the deprecated key
was always read with,
+ // instead of falling back to the seconds Ratis would assume. The warning
must name that resolved
+ // value, so copying it to the current key does not silently reinterpret
it as seconds.
+ logCapturer.clearOutput();
+ OzoneConfiguration bareValueConf = new OzoneConfiguration();
+ bareValueConf.set(DEPRECATED_RETRY_CACHE_KEY, "600000");
+ assertEquals(600_000, retryCacheExpiryMillis(bareValueConf, ratisDir));
+ assertThat(logCapturer.getOutput()).contains(deprecationWarning(600_000));
+
+ // The deprecated key is applied last, so it wins when both are set.
+ OzoneConfiguration bothKeysConf = new OzoneConfiguration();
+ bothKeysConf.set(CURRENT_RETRY_CACHE_KEY, "42s");
+ bothKeysConf.set(DEPRECATED_RETRY_CACHE_KEY, "17s");
+ assertEquals(17_000, retryCacheExpiryMillis(bothKeysConf, ratisDir));
Review Comment:
This describes a possibly undesired behavior change: if both values are set,
previously `ozone.om.ha.raft.server.retrycache.expirytime` was effective, but
now `ozone.om.ratis.server.retry.cache.timeout` will be.
For values other than the default for
`ozone.om.ha.raft.server.retrycache.expirytime` we can easily keep the old
behavior. I understand that currently there is no easy way to distinguish
whether "300s" comes from defaults or explicit configuration. #10926 is adding
methods in `OzoneConfiguration` to help with that.
--
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]
