Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set MaxAttemptsToUpdatePivot to -1 for infinite pivot update trials #8107

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Deeptanshu-sankhwar
Copy link

@Deeptanshu-sankhwar Deeptanshu-sankhwar commented Jan 25, 2025

Fixes Closes Resolves #5992

Changes

  • In ISyncConfig.cs update the default value for MaxAttemptsToUpdatePivot to -1 in an attempt to make the pivot update trials to infinite instead of ~68 years
  • Since _attemptsLeft now begins with a value of -1 and keeps decrementing by 1, updated the condition in PivotUpdator.cs , handle its update condition
  • Since the default value for MaxAttemptsToUpdatePivot change to -1 from positive, updated it in MultiSyncModeSelector.cs, handle both positive and negative value for ShouldBeInUpdatingPivot in MultiSyncModeSelector.cs
  • Thereby updated its test case in MultiSyncModeSelectorTests.Scenario.cs

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Copy link
Contributor

@damian-orzechowski damian-orzechowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes should work both with special value of -1 as well as any value placed via configuration.

@@ -355,7 +355,7 @@ private bool ShouldBeInBeaconHeaders(bool shouldBeInUpdatingPivot)

private bool ShouldBeInUpdatingPivot()
{
bool updateRequestedAndNotFinished = _syncConfig.MaxAttemptsToUpdatePivot > 0;
bool updateRequestedAndNotFinished = _syncConfig.MaxAttemptsToUpdatePivot == -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -873,7 +873,7 @@ public ScenarioBuilder WhenMergeSyncPivotNotResolvedYet()
_syncProgressSetups.Add(
() =>
{
SyncConfig.MaxAttemptsToUpdatePivot = 3;
SyncConfig.MaxAttemptsToUpdatePivot = -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With tests, I'd be cautious to use any settings potentially causing infinite work

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted back the test to set MaxAttemptsToUpdatePivot = 3, so it can test by retrying pivot updates for exactly 3 number of times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor PivotUpdator to be really infinite
2 participants