maytasm edited a comment on pull request #10625: URL: https://github.com/apache/druid/pull/10625#issuecomment-738954717
> I think it would be better to make a new separate parameter that applies a percentage-based limit, and make it mutually exclusive with the existing `maxBytesInMemory`. > > I think it's confusing to have two different modes for the same parameter, and there's no indication in the property name that it can operate in a percentage-based mode. Initially I tried to add another parameter to the ingestionSpec. However, it seems like I will have to change the constructor for `HadoopTuningConfig`, `IndexTuningConfig`, `MemoryParameterOverridingAppenderatorConfig`, `RealtimeAppenderatorTuningConfig`, `RealtimeTuningConfig`, `SeekableStreamIndexTaskTuningConfig`. As a result, you will have to modify/fix many many other class files and test class files (50-70files). Maybe I am missing something? Also, regarding using maxBytesInMemory, values like -1 (no limit) and 0 (default 1/6 max jvm mem) are already "special". I also think there might be more confusion with both parameters.. like when happen if you set both and they conflict (like setting maxBytePercent to 50% when you max java memory is 1gb but maxByteInMemory is also set to 100mb...should it be 500mb or 100mb then). Since this can be a safety net / less user facing / semi hidden hack. If the memory calculation for each row (which we are continuously improving) is accurate then there’s probably no reason to change the default (1/6 max memory) @jon-wei Thanks for the review and lmk 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
