Set MaxAttemptsToUpdatePivot to -1 for infinite pivot update trials#8107
Set MaxAttemptsToUpdatePivot to -1 for infinite pivot update trials#8107Deeptanshu-sankhwar wants to merge 5 commits into
Conversation
damian-orzechowski
left a comment
There was a problem hiding this comment.
I think the changes should work both with special value of -1 as well as any value placed via configuration.
| private bool ShouldBeInUpdatingPivot() | ||
| { | ||
| bool updateRequestedAndNotFinished = _syncConfig.MaxAttemptsToUpdatePivot > 0; | ||
| bool updateRequestedAndNotFinished = _syncConfig.MaxAttemptsToUpdatePivot == -1; |
There was a problem hiding this comment.
This logic needs to work not only with value of -1 - any value can be specified through config. With this logic, any positive value will most likely break syncing.
There was a problem hiding this comment.
Hi @damian-orzechowski , sorry I didn't consider the possibility of any value coming from the config, and the logic shall hold true given any value for MaxAttemptsToUpdatePivot from config. I have now updated the logic where retires happen infinite number of times if MaxAttemptsToUpdatePivot = -1 and finite number of times if MaxAttemptsToUpdatePivot = N, N being a positive number.
| () => | ||
| { | ||
| SyncConfig.MaxAttemptsToUpdatePivot = 3; | ||
| SyncConfig.MaxAttemptsToUpdatePivot = -1; |
There was a problem hiding this comment.
With tests, I'd be cautious to use any settings potentially causing infinite work
There was a problem hiding this comment.
I've reverted back the test to set MaxAttemptsToUpdatePivot = 3, so it can test by retrying pivot updates for exactly 3 number of times.
| private bool ShouldBeInUpdatingPivot() | ||
| { | ||
| bool updateRequestedAndNotFinished = _syncConfig.MaxAttemptsToUpdatePivot > 0; | ||
| bool updateRequestedAndNotFinished = _syncConfig.MaxAttemptsToUpdatePivot != 0; |
There was a problem hiding this comment.
I don't think this is correct. We should be updating pivot with a matching condition to logic in PivotUpdator, that's when -1 or > 0. Could simplify it to >= -1, but probably have to test.
There was a problem hiding this comment.
Hello @damian-orzechowski, I tested this further, and >= -1 actually causes issues in MultiSyncModeSelector. The problem is that it treats -1 and 0 the same, allowing retries when MaxAttemptsToUpdatePivot == 0, which shouldn't happen.
!= 0 correctly allows infinite retries (-1) and limited retries (> 0) while stopping retries when 0. This aligns with logic in PivotUpdator, where 0 explicitly stops retries.
The tests also confirm this behavior, so I think != 0 is the correct choice. Let me know if you see another case I should check!
There was a problem hiding this comment.
Hi, yes, I think I was wrong - we want to keep updating until we reach 0, so the condition '!=0' works for both positive and negative config. One more thing to test would be a different negative value placed via config (e.g. -100). Condition here will evaluate to continue UpdatingPivot state, but logic in PivotUpdator might fail.
|
Closing — changes-requested feedback was never addressed, branch is conflicting/dirty, and pivot-updator behaviour has likely shifted in the ~14 months since. Please reopen against current master if #5992 is still relevant. |
Fixes Closes Resolves #5992
Changes
ISyncConfig.csupdate the default value forMaxAttemptsToUpdatePivotto -1 in an attempt to make the pivot update trials to infinite instead of ~68 years_attemptsLeftnow begins with a value of -1 and keeps decrementing by 1,updated the condition in, handle its update conditionPivotUpdator.csMaxAttemptsToUpdatePivotchange to -1from positive, updated it in, handle both positive and negative value forMultiSyncModeSelector.csShouldBeInUpdatingPivotinMultiSyncModeSelector.csThereby updated its test case inMultiSyncModeSelectorTests.Scenario.csTypes of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?