UOETianleZhang commented on PR #15799: URL: https://github.com/apache/pinot/pull/15799#issuecomment-2902185372
> > I do agree with the general intention, beyond the config package discussed here I personally don't see many other exception cases. > > My concern is that for actively developed config classes (e.g. SchemaConformingTransformer) we'll quickly end up in a situation with dozens of constructors in the classes/hundreds of lines devoted to this. For consumers of the spi I don't think this provides a great experience either - to choose between so many ctors leads to confusion and misuse as users generally choose a simple ctor and don't think about what's required to create the object they need. > > It's also cumbersome and error prone to update all of these for each change. Will contributors expected to test each new constructor? It promotes poor practice such as using Map<String, String> for configs (e.g. FieldConfig properties) since it works around this new limitation. > > What do you think about exempting configs (again, just JSON configs), but also adding builders to the exempted configs such that the spi can be consumed in a way that constructor changes to these classes do not require any code change. IIUC that would prevent the build breaking changes you refer to, and also allow for simpler development process/better maintainability in the long term. > > @itschrispeck Thank you for providing your thoughtful feedback. We are having an internal discussion and will respond later. > > I’m also drafting a complementary proposal and will open a doc/issue later this week or early next week for broader discussion :) cc @siddharthteotia @itschrispeck I believe our discussion can be broken down into three key questions. 1. **Best practices for developing SPI config classes.** I agree with your idea to apply the builder pattern. From a compatibility standpoint, we should recommend that config classes in the SPI either adopt a builder. Or alternatively, default constructor with setters can also work. (depends on developer's preference) 2. **Review process for valid SPI changes (e.g., deprecations).** I already have a proposal and it is under internal review. Will share it later this week/next week. The idea is to allow intentional changes once they have been aware of by consumers and approved by the community, while keeping preventing arbitrary SPI breakages. 3. **Exempting config classes from compatibility checks.** Although config classes effectively act as JSON wrappers, several—most notably TableConfig—have been major contributors to our build failures due to their wide usage and frequent changes. For that reason, I’d prefer to keep all config classes under the compatibility rules. If needed, developers can either adopt the patterns from point 1 or request an override via the process that will be included in the doc in point 2. Looking forward to your thoughts! -- 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]
