markap14 commented on pull request #4554: URL: https://github.com/apache/nifi/pull/4554#issuecomment-754208111
I don't think we can accept this change into the codebase. It has the potential to break a lot of things. Any component that previously declared a property to reference a RecordLookupService would no longer be able to use this service, and that component would become invalid. And any component that declared a property as a LookupService that previously worked as expected could start to behave very differently. Consider that a REST service previously had a response that had 3 records, and it just returned the first one. Now it returns 3. That can result in all kinds of problems in the users' data. However, I can understand the desire to support returning multiple Records. So we could either have a separate implementation (with shared, common code), or we could add a property that determines what to do if multiple records are returned (defaulting to return the first record in order to maintain backward compatibility). Adding the property comes with 2 downsides, though. (1) It means that the Controller Service still would no longer be a RecordLookupService but just a `LookupService<List<Record>>`. This is probably ok, though. I don't think we even have anywhere in the codebase where we expect a RecordLookupService explicitly to be configured. (2) The default really should be to return multiple records, not to return the first and ignore the rest. But defaulting to that would change the default behavior in a very unexpected way for some users. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
