hevinhsu commented on PR #10911: URL: https://github.com/apache/ozone/pull/10911#issuecomment-5139647218
Thanks @F64116045 for the patch! The change looks good to me. I also found that HDFS [`DatanodeManager`](https://github.com/apache/hadoop/blob/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java#L289-L291) instantiates the configured mapping directly. I noticed a few details that I’d like to confirm with the maintainers: 1. Since HDDS-1663, SCM has automatically wrapped non-cached mappings with `CachedDNSToSwitchMapping`. After removing the wrapper, caching and input handling will become the responsibility of the configured implementation. I’m wondering if this could affect existing custom mappings that rely on the current behavior, for example, mappings that expect normalized IP addresses or repeated lookups to be cached. Is this compatibility change expected and acceptable? 2. I noticed that OM has similar wrapper logic in [`KeyManagerImpl`](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java#L388-L390). Should SCM and OM follow the same behavior here? If OM should also use the configured mapping directly, should we handle it in this PR or track it separately? If this behavior change is accepted, I think we should also raise a follow-up issue to update the corresponding topology documentation in [`apache/ozone-site`](https://github.com/apache/ozone-site). -- 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]
