Skip to content

Add comments to explain LastSyncVersion selection #358

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 20, 2022
Merged

Add comments to explain LastSyncVersion selection #358

merged 1 commit into from
Sep 20, 2022

Conversation

@AmeyaRele
Copy link
Contributor

@AmeyaRele AmeyaRele commented Sep 16, 2022 •
edited
Loading

Closes #306

Read up on this in the design doc and this is what I understand for the decision of new last sync version.

  • Since the batch of changes are acquired in ascending order. With multiple version numbers in the batch, the other changes with version numbers less than the highest version number should either be already processed or being leased by other workers, which is why last sync version is updated to second highest version number.
  • In the query where the last sync version is actually updated in the GlobalState Table it checks if there are any unprocessed changes with version numbers lower than new last sync version. If there are none, every change with version number <= new last sync version is cleared from the Leases Table.
  • It is ensured that a worker lagging behind won't update the last sync version to a lower version due to condition of updating only when @current_last_sync_version < {newLastSyncVersion}.
  • Initially the last sync version is set in the GlobalState Table using CHANGE_TRACKING_MIN_VALID_VERSION({userTableId})

Have added comments related to this in the PR.

@AmeyaRele AmeyaRele linked an issue Sep 16, 2022 that may be closed by this pull request
Explain the basis for last sync version selection in comment #306
Closed
@AmeyaRele AmeyaRele merged commit d36422f into triggerbindings Sep 20, 2022
@AmeyaRele AmeyaRele deleted the ameyarele/Explain-LastSyncVersion branch September 20, 2022 05:41
Charles-Gagnon added a commit that referenced this pull request Sep 23, 2022
@Charles-Gagnon @lucyzhang929
@JatinSanghvi @AmeyaRele @MaddyDev
Sync LA release with latest from triggerbindings feature branch (#366)
08e44b2 
* Don't publish TSA results on PRs (#327)

* Don't publish TSA results on PRs

* fix

* Add more debug logging (#328)

* add debug logging

* pr comments + fix test

* log queries

* Add benchmark project for performance tests (#294)

* add benchmark project

* fix logger

* fix logger

* copy functions to benchmark output folder

* cleanup

* move setup methods to base

* add output binding perf

* clean up

* add README

* fix readme

* add link to integration tests

* typo

* cleanup + comments

* address pr comments

* fix readme

* fix integration test base

* pr comments

* Fix localization/serialization issues (#329)

* Fix localization issues

* split tests & cleanup

* Replace file copy with itemgroup

* Add policheck and var for uploading to TSA (#332)

* Add policheck

* Upload policheck to TSA codebase

* Try globbing files

* Scan all

* Only upload when var set

* post analysis

* Only on windows

* break on warn

* testing

* re-enable and break on error

* Update notification alias

* Fix local build errors (#334)

* Fix local build errors

* Update comments

* Fix log error not printing out primary key name (#335)

* Don't run Roslyn analyzers on PRs (#336)

* Don't run Roslyn analyzers on PRs

* fix

* Do not process rows if lease acquisition fails (#339)

* Fix README link (#342)

* Escape special characters in Readme file (#346)

* Wait for error inside `StartFunctionsHostAndWaitForError` method (#349)

* Add test for multiple triggered functions (#343)

* Add retry when attempting to release leases (#340)

* Add retry when attempting to release leases

* Address comments

* Address more comments

* Change loop structure according to comment

* sql version telemetry data from input bindings (#337)

* connection info telemetry data frm input bindings

* capture convert event after collecting connection

* remove event

* first collect telemetry

* fix tests

* make convert type optional

* update xml

* add telemetry service

* clean up

* remove ITelemetryService

* undo changes

* merge conflict

* missed merge conflict

* extra line

* add null check

* capture telemetry after opening connection

* correct connection

* move telemtery after connection.OpenAsync

* open connection on initialize

* add xml

* address comments

* refactor test

* add xml comment

* update xml

* add check back

* add connection state check

* remove explicit checks

* add isDisposed

* add comments

* revert isDisposed changes

* add comment with github issue link

* Refactor locking logic in class `SqlTableChangeMonitor` (#357)

* cherry-pick c5a746f

* Merge commit '76575f29e964f3487684b84e279444279296f5c2' into chgagnon/mergeFromMain1

# Conflicts:
#	src/SqlAsyncCollector.cs
#	test/GlobalSuppressions.cs

* Add comments to explain LastSyncVersion selection (#358)

* Fix

* Move test scripts

* Constructor cleanup (#362)

* Initialize vars at declaration

* Remove extra ;

* More cleanup

* Remove configuration

* Add configuration for batch size/polling interval (#364)

* Add configuration for batch size/polling interval

* PR comments

Co-authored-by: Lucy Zhang <luczhan@microsoft.com>
Co-authored-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
Co-authored-by: AmeyaRele <35621237+AmeyaRele@users.noreply.github.com>
Co-authored-by: Maddy <12754347+MaddyDev@users.noreply.github.com>
chlafreniere pushed a commit that referenced this pull request Sep 20, 2024
@Charles-Gagnon @lucyzhang929
@JatinSanghvi @AmeyaRele @MaddyDev
Sync LA release with latest from triggerbindings feature branch (#366)
78328c6 
* Don't publish TSA results on PRs (#327)

* Don't publish TSA results on PRs

* fix

* Add more debug logging (#328)

* add debug logging

* pr comments + fix test

* log queries

* Add benchmark project for performance tests (#294)

* add benchmark project

* fix logger

* fix logger

* copy functions to benchmark output folder

* cleanup

* move setup methods to base

* add output binding perf

* clean up

* add README

* fix readme

* add link to integration tests

* typo

* cleanup + comments

* address pr comments

* fix readme

* fix integration test base

* pr comments

* Fix localization/serialization issues (#329)

* Fix localization issues

* split tests & cleanup

* Replace file copy with itemgroup

* Add policheck and var for uploading to TSA (#332)

* Add policheck

* Upload policheck to TSA codebase

* Try globbing files

* Scan all

* Only upload when var set

* post analysis

* Only on windows

* break on warn

* testing

* re-enable and break on error

* Update notification alias

* Fix local build errors (#334)

* Fix local build errors

* Update comments

* Fix log error not printing out primary key name (#335)

* Don't run Roslyn analyzers on PRs (#336)

* Don't run Roslyn analyzers on PRs

* fix

* Do not process rows if lease acquisition fails (#339)

* Fix README link (#342)

* Escape special characters in Readme file (#346)

* Wait for error inside `StartFunctionsHostAndWaitForError` method (#349)

* Add test for multiple triggered functions (#343)

* Add retry when attempting to release leases (#340)

* Add retry when attempting to release leases

* Address comments

* Address more comments

* Change loop structure according to comment

* sql version telemetry data from input bindings (#337)

* connection info telemetry data frm input bindings

* capture convert event after collecting connection

* remove event

* first collect telemetry

* fix tests

* make convert type optional

* update xml

* add telemetry service

* clean up

* remove ITelemetryService

* undo changes

* merge conflict

* missed merge conflict

* extra line

* add null check

* capture telemetry after opening connection

* correct connection

* move telemtery after connection.OpenAsync

* open connection on initialize

* add xml

* address comments

* refactor test

* add xml comment

* update xml

* add check back

* add connection state check

* remove explicit checks

* add isDisposed

* add comments

* revert isDisposed changes

* add comment with github issue link

* Refactor locking logic in class `SqlTableChangeMonitor` (#357)

* cherry-pick 6f23a5a

* Merge commit 'd594efe88f0f7d32e4dcd31faad22885291d4902' into chgagnon/mergeFromMain1

# Conflicts:
#	src/SqlAsyncCollector.cs
#	test/GlobalSuppressions.cs

* Add comments to explain LastSyncVersion selection (#358)

* Fix

* Move test scripts

* Constructor cleanup (#362)

* Initialize vars at declaration

* Remove extra ;

* More cleanup

* Remove configuration

* Add configuration for batch size/polling interval (#364)

* Add configuration for batch size/polling interval

* PR comments

Co-authored-by: Lucy Zhang <luczhan@microsoft.com>
Co-authored-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
Co-authored-by: AmeyaRele <35621237+AmeyaRele@users.noreply.github.com>
Co-authored-by: Maddy <12754347+MaddyDev@users.noreply.github.com>
PBBlox pushed a commit to PBBlox/azure-functions-sql-extension that referenced this pull request Apr 6, 2025
@AmeyaRele
Add comments to explain LastSyncVersion selection (Azure#358)
6877aeb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Charles-Gagnon Charles-Gagnon Charles-Gagnon approved these changes

@lucyzhang929 lucyzhang929 Awaiting requested review from lucyzhang929

@chlafreniere chlafreniere Awaiting requested review from chlafreniere

@MaddyDev MaddyDev Awaiting requested review from MaddyDev

@JatinSanghvi JatinSanghvi Awaiting requested review from JatinSanghvi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Explain the basis for last sync version selection in comment

3 participants

@AmeyaRele @Charles-Gagnon