zhangyue19921010 edited a comment on pull request #10524: URL: https://github.com/apache/druid/pull/10524#issuecomment-759552196
> 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. ---------------------------------------------------------------- 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: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
