suneet-s commented on pull request #10524: URL: https://github.com/apache/druid/pull/10524#issuecomment-759648807
> > This sounds like a great capability! I haven't read the implementation in detail yet, but I see that not many tests were added. Can you talk about what tests would be needed to safeguard us from regressions. Do we have the needed test infrastructure to write integration tests for these scenarios? If not, what's missing? > > Hi @suneet-s Thanks for your response. This feature has an important property named `enableDynamicAllocationTasks` which is false as default value. If users set it false or ignore this property, this feature will be disable and have no impact on indexing service. All the ingest related UTs or ITs are passed which can ensure this conclusion. > > If anything goes wrong caused by this feature, we can solve it easily. Just set it false. > > The core design of this PR is to do a little change of supervisor behavior in overlord. Now overlord can build a `dynamicAllocationNotice` itself to change the task number. So that I just modify `SeekableStreamSupervisorStateTest` to test and prove that Supervisor works fine including start, stop and so on when setting `enableDynamicAllocationTasks` true and enable this feature. Also more tests(maybe something monitor the change of task counts?) can be added if necessary and the existing architecture is sufficient to meet this requirement. @zhangyue19921010 This is good that there is a feature flag and it's disabled by default. But if we announce this functionality, users would expect it to work, and continue to work on upgrades. We should build a test plan so that we can test enabling this feature with every new release of Druid. As we test experimental features like Indexers - having some integration tests here will make it easier for us to verify that this new functionality continues to work if a user switches to use Indexers instead of middle managers for example. ---------------------------------------------------------------- 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]
