Eason09053360 commented on PR #11096: URL: https://github.com/apache/ozone/pull/11096#issuecomment-5413713798
> Thanks for the patch! @Eason09053360 Quick question on the migration path. The old code seems read this key as milliseconds, but through the alias, Ratis falls back to its own unit, so I wasn't sure what happens to a cluster that set a bare `600000`. Would that end up much longer than intended? And do operators actually get a deprecation warning here, given how the value is read? Thanks @chihsuan, you're right on both counts. Units. The old path read the key with TimeUnit.MILLISECONDS as the fallback unit, so a bare 600000 meant 10 minutes. Through the alias the raw string goes straight to Ratis, which falls back to seconds (RetryCache.EXPIRY_TIME_DEFAULT = 60s) — so 600000 becomes ~6.9 days. 600000ms and 300s are unaffected. My test used 17s, which carries a unit, so it missed this. About Deprecation warning. There is none. Hadoop logs it only on get()/set() of the old key, and loading it from XML just copies the value across silently. This patch removed the only get(), so nothing fires. I prefer Restore setRaftRetryCacheProperties() but move the call below getOMHAConfigs(conf).forEach(properties::set), applied only when the old key is explicitly set. That reverses the override behind the original bug, keeps millisecond semantics, and the conf.get() makes the warning fire. I'll keep the DeprecationDelta for the "Instead, use ..." message and add a bare-value test case. -- 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]
