kfaraz commented on PR #16442: URL: https://github.com/apache/druid/pull/16442#issuecomment-2110205615
Thanks for the improvement, @YongGang . It makes sense to not create new tasks if they are going to inevitably fail. Even though this PR is still in draft, I have some suggestions/queries that you could try to incorporate early in the design: - If the supervisor is not going to launch tasks, would it make sense to just move the supervisor to an `idle state` rather than introduce a new state? - Rather than introduce a new config, why not reuse the existing `idleConfig` (https://druid.apache.org/docs/latest/ingestion/kafka-ingestion/#idle-configuration). That was also introduced keeping in mind cost concerns and unnecessary task launches. We could just add a new field to `idleConfig` which dictates the supervisor to become idle if `maxAllowedParseExceptions` is violated. - It should be easy for the user to figure out __why__ the supervisor was moved to the idle state. - Based on the PR description, this seems fairly tied in to the `maxAllowedParseExceptions` but the code changes don't seem to be using `maxAllowedParseExceptions` anywhere. Is there any other scenario where we would want to make the supervisor idle? - What if `maxAllowedParseExceptions` keeps getting violated for some partitions and not for others. The task launched for the former partitions would always fail but should we become idle in this case or just not launch tasks to read from the bad partitions? - I am not sure if the "no progress" check is really going to be effective or if it is even desirable. There could be other reasons behind not making progress and not all of them would qualify for making the supervisor idle. - The `StateManager.markRunFinished()` now accepts a `Boolean` which is almost always passed as `null` except in one or two places. Please consider adding a separate method instead. -- 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]
