jon-wei commented on pull request #10625: URL: https://github.com/apache/druid/pull/10625#issuecomment-739092992
> 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? I feel like that's unfortunate, but I don't think we should generally design a parameter around avoiding a tedious implementation, the focus should be on user experience. Maybe we need to take a look at consolidating these config objects somehow to make this kind of change easier... > Also, regarding using maxBytesInMemory, values like -1 (no limit) and 0 (default 1/6 max jvm mem) are already "special". I personally find that less "unusual" than the 1-100 range being a percent, since -1, 0, don't have much meaning when interpreted as a size limit, while in theory you could have a 1-100 byte limit (though I think it's fair point that this would be impractical and no one would actually do that). > 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). If both were set, I was thinking it should throw an error indicating that the parameters conflict. > 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) I could see this as justifiable cover for repurposing a range of maxBytesInMemory in a kind of hacky way, but I still think it'd be nicer as a new parameter. I don't think I would block the PR on that basis though. ---------------------------------------------------------------- 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]
