supervisor: make rejection periods work with stopTasksCount#17442
supervisor: make rejection periods work with stopTasksCount#17442suneet-s merged 13 commits intoapache:masterfrom
Conversation
suneet-s
left a comment
suneet-s
left a comment
There was a problem hiding this comment.
This implementation of refreshing the min / max times every 10 min is a change in behavior, so I think the person submitting the supervisor spec needs to opt in to this new behavior which doesn't appear possible in the current approach.
An alternative approach to consider is to update the IOConfig to include a "refresh interval" field that defaults to infinity that would refresh the min / max times in the IOConfig.
The tasks could then update their min / max times every duration just by adding the refresh interval so we don't need to worry about clock skew across the peons.
Since this patch is meant to just deal with stopTaskCount - have you considered refreshing the min / max times every task duration to mimic the existing behavior if stopTaskCount was not set (aka every task duration the min/max times across all peons are updated)
| @JsonProperty("lateMessageRejectionPeriod") Duration lateMessageRejectionPeriod, | ||
| @JsonProperty("earlyMessageRejectionPeriod") Duration earlyMessageRejectionPeriod, |
There was a problem hiding this comment.
These new fields need to be in addition to the old fields to ensure a newer version of the overlord works with older versions of the indexing tasks. But more importantly, in the case of a rollback - an older overlord needs to work with a newer version of indexing services
ac9817
force-pushed
the
rejection-periods
branch
from
e8bef0c to
1377975
Compare
|
@suneet-s Thanks for the review, I've changed the patch to just update the min/max times once every task duration. So this also avoids all those version problems. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ac9817
changed the title
suneet-s
left a comment
suneet-s
left a comment
There was a problem hiding this comment.
I have not reviewed the tests yet.
For this change, we should also include docs for what this new parameter is in the IOConfig
ac9817
force-pushed
the
rejection-periods
branch
from
6877bc7 to
bba9486
Compare
suneet-s
left a comment
suneet-s
left a comment
There was a problem hiding this comment.
Thanks for this change. I have one last small ask to add some logging. Also, the docs in https://github.com/apache/druid/blob/master/docs/ingestion/supervisor.md#io-configuration mention that there are problems if the message rejection period is set too low - can that caveat be removed now?
The CI failure seems like it's ok to overrule since it is complaining about branch coverage about moved code and the code that is missing tests is debug logging.
|
Overruling CI. I had to re-trigger a bunch of failed ITs, and now they have all passed. The only remaining failures are code coverage. |
| maxMessageTime = ioConfig.getMaximumMessageTime().or(DateTimes.MAX); | ||
|
|
||
| if (ioConfig.getRefreshRejectionPeriodsInMinutes() != null) { | ||
| Execs.scheduledSingleThreaded("RejectionPeriodUpdater-Exec--%d") |
There was a problem hiding this comment.
Who closes this exec service ?
There was a problem hiding this comment.
This seems to be creating on thread for each supervisor. Seems a bit much no ? Can we use a common exec service for this ?
There was a problem hiding this comment.
This seems to be creating on thread for each supervisor. Seems a bit much no ? Can we use a common exec service for this ?
+1
There was a problem hiding this comment.
this is creating thread on task not on supervisor ?
6 participants
Description
In cases where
stopTasksCountis configured which makes streaming tasks run much longer (in some cases) than the configured task duration, early/late rejection periods are not correctly implemented since they are configured during the task startup time in IOConfig. Instead, this PR propagates updates the min/max times once every task duration.Key changed/added classes in this PR
SeekableStreamIndexTaskRunnerSeekableStreamIndexTaskIOConfigThis PR has: