snleee commented on pull request #7368:
URL: https://github.com/apache/pinot/pull/7368#issuecomment-930628606
@Jackie-Jiang When I made the change, I checked on the leader status part.
Currently, we have 2 different concept of `isLeader`.
1. `isLeader` boolean value to invoke `nonLeaderCleanUp()` for
`PinotTaskGenerator`.
- We invoke `nonLeaderCleanUp()` when we invoke the task schedule using
API because we pass `isLeader=false` (e.g. `PinotTaskManager:scheduleTask() -
L459`.
- We do not invoke `nonLeaderCleanUp()` when the task schedule is
triggered via periodic task because we hardcoded `isLeader=true` (e.g.
`PinotTaskManager:processTables() - L485`).
- So, `isLeader` value here is a bit misleading because it is actually not
related to the lead controller for the table. `nonLeaderCleanUp()` simply gets
called when the task schedule is called by API call (not periodic task).
2. Lead controller for a table via Helix controller separation.
- `LeadControllerManager.isLeaderForTable()` will tell us whether the
current controller is the leader for the given table.
So, here are the things that I considered to handle for metrics:
1. We need to reset the metric when the current controller is no longer the
leader for this particular table
- I check it via `leadControllerManager.isLeaderForTable()` and clean up the
metric.
2. Behavior for API call vs periodic task
- In case we manually invoke the API, I wanted to keep the leader controller
for this table to emit metrics since it's fine to have ~1hr delay for these
metrics to get bumped up. It's more confusing if we see metrics are starting to
get emitted from multiple controllers for a single table. Given that the manual
trigger will be a rare case, I think that it's fine with updating the metrics
only from the periodic tasks.
- I get your point that the current code will clean up the metrics because
`candidateMergeTables` has only 1 table. This is not the intended behavior. I
will make the adjustment. I want to make the entire metrics to be `No OPS` in
case of API schedule invoke.
3. Handle the case where the merge task configs get removed or `mergeLevel`
is removed
- For periodic task cases, this should be handled by the `cleanUpCandidate`
check. I will address the case for the API invoke case.
- I missed the `mergeLevel` change part. I will address it.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
