kfaraz commented on PR #16512: URL: https://github.com/apache/druid/pull/16512#issuecomment-2137541545
@georgew5656 , thanks for the changes! It makes sense to fail early on large payloads with a clear error message, but I have some concerns. - Such tasks already fail on insertion into the metadata store. Do you think the current error message in this case is not clear enough? Instead of a new check/config, should we try to improve the existing error message by maybe catching it and throwing a cleaner `DruidException`? - Or do you feel that there are other challenges with large task payloads too (say, Overlord facing a memory crunch)? - The only problem with introducing this new config with a default of 64MiB is that some clusters would already be using large task payloads and would have set their `max_allowed_packet` to large enough values that allow such payloads. And if we go with a default that is large enough to accommodate all clusters, then we would probably defeat the purpose of the config itself. - There is also the MSQ case to consider where the user just submits a SQL and the task payload is actually generated by the system. How does the user control the task payload size in that case? Maybe we don't fail in the case the of msq task types? One compromise could be to keep the check and just _warn_ about large task payloads for the time being, with a log message and maybe also an alert event. Let me know what you think! -- 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]
