okumin commented on code in PR #5882: URL: https://github.com/apache/hive/pull/5882#discussion_r2259109607
########## standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java: ########## @@ -33,21 +33,23 @@ /** * Catalog & servlet factory. + * <p>This class is derivable on purpose; the factory class name is a configuration property, this class + * can serve as a base for specialization.</p> Review Comment: Thanks. Can we separate it from this PR? The title of this PR is "Fixing cache handling for REST catalog" and your point does not belong to the original interest of this PR. I suppose we need to clarify the three points to justify the new ticket. One, we need clear use cases to create a subclass. I don't rule out the potential use cases, but "potential" does not justify the change; Every class is likely to have a potential use case to be inherited. Two, we will need to be confident that `extends HMSCatalogFactory` is the best method to satisfy the use case. We are recently removing inheritance[1] because we failed to manage inflated complexity using inheritance. I suppose we would likely be more successful with YAGNI in this case; We can add the best abstraction, which will have real users. I can approve and merge this PR if we can separate the problem from this PR. - [1] https://docs.google.com/document/d/1lppPj35fBOOCjGNIouoJCsniMPpyzWx3zcE7kvu0Be4/edit?tab=t.0#heading=h.1upmmt6vhhfa -- 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: gitbox-unsubscr...@hive.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org For additional commands, e-mail: gitbox-h...@hive.apache.org
