Re: [PR] [server] Add off-loop coordinator health cache (mechanism only) [fluss]

["fresh-borzoni (via GitHub)" <gi...@apache.org>]

fresh-borzoni commented on PR #4061:
URL: https://github.com/apache/fluss/pull/4061#issuecomment-5955110796

   @affo thanks, everything from the first round is fixed, and putting the hooks in CoordinatorContext's mutators works really well.
   
   I've been thinking about the tradeoff again, and I think my last review sent you the wrong way. I asked for a minimum interval, and that's where the timers came from. Looking at who actually calls these APIs, I'm not sure caching pays off:
    - Calls are rare. The FIP-41 operator checks health between upgrade steps and waits 30 s before moving on. The readiness probe asks once every 5 s, and only while a pod is starting.
    - Changes come in bursts. During a rolling upgrade, leaders move constantly, so the cache rescans every 200 ms or so. That's more scans than the operator would ever ask for, at the moment the coordinator is busiest. Computing on each call costs one scan per question, nothing in between, and the answer is always current. That's close to what @LiebingYu suggested in #1389: keep the per-server API low-frequency.
   
   So what if #3743 computes the per-server and cluster numbers together in one pass per call? Then the operator gets one consistent view for its gate. We could park this PR until something needs to read health often. If that happens, running totals kept in the mutators, the way offlineBucketCount works, would be cheaper and fresher than timed rescans. 
   
   @morazow, that would mean you don't need to wait for this one.
   
   If we do keep this version, two bugs need fixing first:
    - If refresh() throws, the coordinator stops processing events for good. It's called outside the try/catch in doWork, and ShutdownableThread exits its loop on any exception. A try/catch around is the most simple fix here I guess
   - The 200 ms limit doesn't hold. refresh() only runs when the event loop wakes up, so a change that arrives right after a refresh waits for the next wakeup, which can be up to 5 s later. Capping the poll timeout at the time until the next refresh is due fixes it.
   
   @LiebingYu @swuferhong, does this match how you see 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: issues-unsubscribe@fluss.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org