suneet-s commented on PR #14396: URL: https://github.com/apache/druid/pull/14396#issuecomment-1636199440
> I am +1 on not having this on by default (default would still be the current behavior) and having the config as number of tasks to roll at one time configurable per each supervisor). I am -1 on this PR as a whole due to the lack in unit testing for when the new config is used. I also have a few minor questions. I looked into writing tests for this and struggled. SeekableStreamSupervisor is a huge abstract class, and the part that needs testing uses `DateTime.now` which isn't very testable, so I would need to refactor the class to use a Clock to simulate time passing by. It feels like trying to make this class testable introduces more risk. I even looked at adding an integration test, but it seemed like that would be flaky and take a long time to run as we would have to prove that there was no more than 1 task handing over at a time. If there are any suggestions or pointers on how to write tests for this class, I'm open to trying it out. Otherwise, I think the fact that this is disabled by default and undocumented provides the safety net we need that it won't break for Druid 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. 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]
