nevzheng commented on PR #10975: URL: https://github.com/apache/gravitino/pull/10975#issuecomment-5012128559
**Optional follow-up: `GravitinoClientBase` architecture audit** While tracing this change, I did a small SOLID audit of the existing client architecture. This is pre-existing and is not intended to block this PR. `GravitinoClientBase` currently has several independent reasons to change: Gravitino domain/version behavior, metalake operations, HTTP transport construction and configuration, and transport lifecycle. That is an SRP concern. Although the class stores a `RESTClient` abstraction, it directly constructs the concrete `HTTPClient`. This means the high-level client still depends on and configures its concrete transport, which is a DIP concern. The fact that adding one HTTP feature requires changes across the base class, both concrete clients, their constructors, and their builders also creates Open/Closed pressure. I do not see enough evidence here to claim specific LSP or ISP violations, so I would not characterize this as violating every SOLID principle. The concrete findings are SRP, DIP, and the resulting difficulty of extending transport behavior without modifying the client hierarchy. A possible cleanup direction would be to: - introduce a shared client factory/composition point that builds the transport; - inject a `RESTClient` or `RESTClient` factory into the Gravitino clients; - keep TLS, headers, authentication, proxy settings, and other HTTP concerns in `HTTPClient.Builder`; and - leave the Gravitino client classes responsible only for Gravitino behavior. If the maintainers are interested, I would be happy to help turn this into a focused follow-up cleanup. -- 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]
