mlevkov commented on PR #4064:
URL: https://github.com/apache/iggy/pull/4064#issuecomment-5624232699

   Fixed at `8e3e9299e`, rebased onto master first. 215 unit tests and 9 
connectors
   integration tests green.
   
   You were right, and the second half of your sentence is the part I had 
missed.
   `set_error` moved the status out of `Running` without moving the gauge, so 
the
   increment was never given back. That is why the count reached 2 rather than
   merely being set late, and it also means the loop's later `Stopped` could not
   correct it: by then the old status was `Error`, and neither branch of the 
gauge
   rule fires.
   
   `SourceDetails::apply_status` now owns the transition and the gauge move
   together. `update_status` takes the lock and delegates, `set_error` 
delegates and
   then sets its message, and `start_connector` calls it in the same hold as 
the id
   record. The trailing unconditional report is gone.
   
   The regression test puts the error between the two `Running` reports. 
Removing
   the gauge move from `set_error` leaves the gauge reading 1 where it should 
read
   0, and the test fails on that. You were right that the consecutive-`Running`
   test cannot reach this: there the second report finds the status already
   `Running`, so nothing crosses and no arithmetic runs.
   
   One thing I did not change and one I could not test.
   
   `SinkManager::set_error` has the same gauge asymmetry. It is outside this 
PR's
   two files, and the sink consume loop never reports status at all, so the 
shape of
   the fix differs. Happy to fold it in if you would rather it went here.
   
   The ordering itself has no test. `start_connector` needs a dlopened 
container, so
   what holds it is that the transition sits in the same lock hold as the id 
record
   with no await between them. The second test pins `set_error`'s observable
   contract and its comment says plainly what it does not pin, since the 
message is
   assigned after the transition and a mutant widening the clear survives.
   
   The rebase moved every sha, so the ones in my earlier comment on this PR are
   stale; I have refreshed them there.
   


-- 
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]

Reply via email to