Skip to content

Add retry when attempting to release leases #340

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 4 commits into from
Sep 14, 2022
Merged

Add retry when attempting to release leases #340

merged 4 commits into from
Sep 14, 2022

Conversation

@AmeyaRele
Copy link
Contributor

@AmeyaRele AmeyaRele commented Sep 9, 2022

Closes #282

Charles-Gagnon
Charles-Gagnon approved these changes Sep 9, 2022
View reviewed changes
JatinSanghvi
JatinSanghvi reviewed Sep 10, 2022
View reviewed changes
src/TriggerBinding/SqlTableChangeMonitor.cs Outdated
break;
}
catch (Exception ex2)
catch (Exception ex) when (retryCount < MaxRetryReleaseLeases)
Copy link
Contributor

@JatinSanghvi JatinSanghvi Sep 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, we should log the error and attempt to roll back the transaction even for the last retry. Am I missing something here? The for-block can be placed outside the outer-most try-block. Adding @Charles-Gagnon for his opinion on this.

Copy link
Contributor

@Charles-Gagnon Charles-Gagnon Sep 12, 2022

Choose a reason for hiding this comment

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

We should definitely be trying to rollback on the last exception. The error and exception event being logged though I think I would prefer to keep separate as they are - we want to make it clear that it failed all the retry attempts and is now going to move on (and from telemetry purposes I want a separate event in case we change the number of retries later - so that we can easily pick out the number of times that a user hit the worst-case scenario where the retries all failed).

So what I'd suggest is to :

  1. Remove the when from this catch
  2. Have an if in here that changes what is logged/tracked based on whether it's the last attempt or not
  3. Remove the outer catch
  4. You should be able to just remove the finally as well and have it run after the loop - since we'll be catching and handling all the exceptions

JatinSanghvi
JatinSanghvi reviewed Sep 10, 2022
View reviewed changes
@JatinSanghvi JatinSanghvi linked an issue Sep 10, 2022 that may be closed by this pull request
Add retry if releasing the lease fails #282
Closed
JatinSanghvi
JatinSanghvi reviewed Sep 13, 2022
View reviewed changes
@AmeyaRele AmeyaRele force-pushed the ameyarele/retry-release-leases branch from 2d32f21 to ad18efb Compare September 13, 2022 08:45
@AmeyaRele AmeyaRele merged commit f42d273 into triggerbindings Sep 14, 2022
@AmeyaRele AmeyaRele deleted the ameyarele/retry-release-leases branch September 14, 2022 05:24
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 retry when attempting to release leases (Azure#340)
c7ec0f0 
* Add retry when attempting to release leases

* Address comments

* Address more comments

* Change loop structure according to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MaddyDev MaddyDev MaddyDev approved these changes

@JatinSanghvi JatinSanghvi JatinSanghvi approved these changes

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

@chlafreniere chlafreniere Awaiting requested review from chlafreniere

+1 more reviewer

@lucyzhang929 lucyzhang929 lucyzhang929 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

trigger

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add retry if releasing the lease fails

6 participants

@AmeyaRele @MaddyDev @JatinSanghvi @lucyzhang929 @Charles-Gagnon