andrewgrantcflt commented on PR #23245: URL: https://github.com/apache/kafka/pull/23245#issuecomment-5430821368
> Thanks for the change @andrewgrantcflt. Overall I think the approach of making this hard coded limit into an internal config makes sense to me. > > One note in `ScramControlManager#alterCredentials` L230: > > ``` > List<ApiMessageAndVersion> records = new ArrayList<>(); > ``` > > Can we make this a `BoundedList` with the same `maxRecordsPerBatch` we have enforced in the other managers for user operations? > > Another place where we can exceed this limit is when registering a broker whose previous shutdown was not clean. However, I think this case is different in that generating "too many" records should not prevent a broker from registering after unclean shutdown. I would argue we should prevent user operations that do this (e.g. createTopics) to prevent unnecessary memory strain on the active controller. What do you think? Thanks @kevin-wu24! I think it makes sense to update `ScramControlManager` to also use a `BoundedList`. I'm not 100% sure though so maybe I create a ticket and do that as a follow-up? There may be other places we also ought to use a `BoundedList`. I just want to keep the changes in this PR focused on making the limit configurable and not other behavior changing ones as much as possible. For the registration related comment, my guess is in some places we'd never expect more than 10,000 records to be generated. For example in the broker registration code I'm guessing we don't register or unregister more than 10,000 brokers at a time :) -- 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]
