replace read async with read of SqlReader for perf #904

MaddyDev · 2023-08-09T20:26:10Z

Fixes #903

To preemptively address the below issues, we followed the fix made in sqltools service (microsoft/sqltoolsservice#2161)

dotnet/SqlClient#593
dotnet/SqlClient#44

[Trigger] Add release branch to PR build triggers (#734)

This reverts commit 9713259.

* add support for jobject and js, ps, python samples * use utils JsonSerializeObject + comment * Update src/TriggerBinding/SqlTriggerBindingProvider.cs Co-authored-by: Charles Gagnon <chgagnon@microsoft.com> * fix unit test + pylint --------- Co-authored-by: Charles Gagnon <chgagnon@microsoft.com>

)

* Add link to privacy statement * Add privacy link to output message * add to output message * Fix typos (cherry picked from commit a9565f7)

* add sqltrigger attribute * add SqlChange type * enable oop test * add check * fix the null reference * use GetLogger

[Trigger] Add Privacy Statement to README and telemetry message (#751)

* enable debug logging for ps and python samples * enable debug logs for js samples

* add js, py, ps ProductsTriggerWithValidation test * fix test + missing file

* Further clarifications to trigger retry docs * Update description

* add more oop test samples * update logger

(cherry picked from commit d6ea6d7)

[Trigger] Filter out more default telemetry properties

# Conflicts: # README.md # builds/azure-pipelines/build-release.yml # src/Telemetry/Telemetry.cs

* add SQLTrigger annotation to java library * remove default values * use enum for commandtype

* add js, py, ps ProductsTriggerWithValidation test * fix test + missing file * add tests * remove extra comments + powershell compress * update comment

[Trigger] Merge latest from main

* Refactor SqlTriggerListener scaling to SqlTriggerMetricsProvider and SqlTriggerScaleMonitor * Create SqlTriggerTargetScaler * Refactor unit tests * Refactor to include common queries for scaling and listener class to SqlTriggerUtils * Add doc comments for scaling classes * Update src/TriggerBinding/SqlTriggerTargetScaler.cs Co-authored-by: Charles Gagnon <chgagnon@microsoft.com> * Fix log statement * Update WebJobs package * Update nuget.config * Address review comments * Address review comments pt2 * Update src/TriggerBinding/SqlTriggerTargetScaler.cs Co-authored-by: Charles Gagnon <chgagnon@microsoft.com> * Address comments, test failures * Fix packages lock file * Fix error message * Address comments and test failures * Apply suggestions from code review Co-authored-by: Charles Gagnon <chgagnon@microsoft.com> * Change in documentation * Fix log level --------- Co-authored-by: Charles Gagnon <chgagnon@microsoft.com>

[Trigger] Merge latest from main

Charles-Gagnon · 2023-08-09T21:45:03Z

@cheenamalhotra This look good to you?

cheenamalhotra · 2023-08-09T22:10:51Z

src/SqlAsyncEnumerable.cs

@@ -112,7 +112,7 @@ private async Task<bool> GetNextRowAsync()
                            this._reader = await command.ExecuteReaderAsync();


Since ExecuteReaderAsync has issues with cancellation - I would recommend changing that to ExecuteReader too. Do you plan to do that separately?

That goes for any Async calls from SqlClient, right? @MaddyDev Make sure you're doing a search all for anything with "Async" in its name so we aren't missing any others.

@MaddyDev You didn't update this one yet, is that on purpose?

Charles-Gagnon · 2023-08-10T16:22:02Z

src/SqlAsyncCollector.cs

@@ -599,7 +599,7 @@ public static async Task<TableInformation> RetrieveTableInformationAsync(SqlConn
                {
                    string getPrimaryKeysQuery = GetPrimaryKeysQuery(table);
                    var cmd = new SqlCommand(getPrimaryKeysQuery, sqlConnection);
-                    using (SqlDataReader rdr = await cmd.ExecuteReaderAsyncWithLogging(logger, CancellationToken.None))
+                    using (SqlDataReader rdr = cmd.ExecuteReaderWithLogging(logger))
                    {
                        while (await rdr.ReadAsync())


What about this ReadAsync (and the one above)?

src/TriggerBinding/SqlTableChangeMonitor.cs

lucyzhang929 · 2023-08-11T12:38:58Z

src/SqlAsyncCollector.cs

                {
                    TelemetryInstance.TrackEvent(TelemetryEventName.TableInfoCacheMiss, props);
                    // set the columnNames for supporting T as JObject since it doesn't have columns in the member info.
-                    tableInfo = await TableInformation.RetrieveTableInformationAsync(connection, fullTableName, this._logger, GetColumnNamesFromItem(rows.First()), this._serverProperties);
+                    tableInfo = TableInformation.RetrieveTableInformationAsync(connection, fullTableName, this._logger, GetColumnNamesFromItem(rows.First()), this._serverProperties);


Should we also remove the Async from the method names? (RetrieveTableInformation)

+1 - for any methods that no longer have await in them we should clean those up

chlafreniere · 2023-08-11T17:44:19Z

How will we measure impact after this change?

Charles-Gagnon · 2023-08-11T18:29:40Z

That would seem to fit nicely into the work we already have planned for perf baselines. @MaddyDev maybe you hold off on getting this in for now (since it's not actively blocking anyone AFAIK) and instead focus on getting the perf stuff figured out first so we can both test the usefulness of that along with validating that this is actually making an improvement?

GeeSuth · 2023-09-18T21:12:44Z

it's that mean the #903 will not fixed?

Charles-Gagnon and others added 30 commits March 15, 2023 09:14

Merge pull request #736 from Azure/chgagnon/syncMain

dcbf3ea

[Trigger] Add release branch to PR build triggers (#734)

Revert "Remove trigger binding for GA release (#732)" (#733)

d323132

This reverts commit 9713259.

vBump release/trigger 2.* -> 3.* (#738)

2a82a61

Fix trigger connection not being retried when connection is closed (#731

9d6988c

)

Add Privacy Statement to README and telemetry message (#751)

9819cea

* Add link to privacy statement * Add privacy link to output message * add to output message * Fix typos (cherry picked from commit a9565f7)

enable trigger oop (#750)

5cce9d6

* add sqltrigger attribute * add SqlChange type * enable oop test * add check * fix the null reference * use GetLogger

Merge pull request #752 from Azure/chgagnon/portTelemetryUpdates

a424e03

[Trigger] Add Privacy Statement to README and telemetry message (#751)

Enable debug logging for Python and PS samples (#754)

febf17d

* enable debug logging for ps and python samples * enable debug logs for js samples

Add docs & logging information for Leases table info (#756)

717fe16

Add ProductsTriggerWithValidation tests for Python, JS, PS (#753)

cc0218f

* add js, py, ps ProductsTriggerWithValidation test * fix test + missing file

Further clarifications to trigger retry docs (#770)

bc87f7e

* Further clarifications to trigger retry docs * Update description

increase timeout to 120 minutes (#776)

e683dce

Enable more tests for OOP trigger (#777)

f035e92

* add more oop test samples * update logger

bump jackson dependency (#782)

bbbe543

Add detailed trigger docs (#780)

5eed998

Filter out more default telemetry properties

e367aa3

(cherry picked from commit d6ea6d7)

Merge pull request #784 from Azure/chgagnon/portTelemetry

90d0ea5

[Trigger] Filter out more default telemetry properties

Merge branch 'main' into chgagnon/mergeMain

33ee38c

# Conflicts: # README.md # builds/azure-pipelines/build-release.yml # src/Telemetry/Telemetry.cs

bring back trigger text

b7a76b8

Fixes

e8fe283

Add SQLTrigger annotation to java library (#783)

d6aa85d

* add SQLTrigger annotation to java library * remove default values * use enum for commandtype

Disable CSX trigger tests

5bdb495

fix regex for java library version (#788)

290fbbc

Adding rest of Trigger integration tests for Python, PS, JS (#763)

b14070a

* add js, py, ps ProductsTriggerWithValidation test * fix test + missing file * add tests * remove extra comments + powershell compress * update comment

Merge pull request #785 from Azure/chgagnon/mergeMain

21837bb

[Trigger] Merge latest from main

Skip more trigger tests for CSX (#791)

621ef15

Merge branch 'main' into chgagnon/mergeMain

c414f9b

Merge pull request #794 from Azure/chgagnon/mergeMain

a13da52

[Trigger] Merge latest from main

MaddyDev requested review from chlafreniere and VasuBhog as code owners August 9, 2023 20:26

Charles-Gagnon requested a review from cheenamalhotra August 9, 2023 21:43

Charles-Gagnon approved these changes Aug 9, 2023

View reviewed changes

cheenamalhotra approved these changes Aug 9, 2023

View reviewed changes

replace ExecuteReaderAsync

92faba7

lucyzhang929 approved these changes Aug 10, 2023

View reviewed changes

Charles-Gagnon reviewed Aug 10, 2023

View reviewed changes

src/TriggerBinding/SqlTableChangeMonitor.cs Outdated Show resolved Hide resolved

MaddyDev added 2 commits August 10, 2023 10:50

remove missed ExecuteReaderAsync

7c5e00f

remove ReadAsync

b1a8847

lucyzhang929 reviewed Aug 11, 2023

View reviewed changes

MaddyDev marked this pull request as draft August 14, 2023 07:44

remove async from updated methods

cd56e02

kevbry mentioned this pull request Aug 16, 2023

Reading large data (binary, text) asynchronously is extremely slow dotnet/SqlClient#593

Open

MaddyDev changed the base branch from release/trigger to main September 22, 2023 18:12

MaddyDev added 6 commits September 22, 2023 11:18

Merge branch 'main' into maddy/replaceReadAsync

3df7449

merge main

714c052

fix perf uml

4b86623

resolve doc conflicts

fe16e4d

resolve remaining conflicts

6173667

fix incomplete merge

7e904b1

MaddyDev marked this pull request as ready for review October 6, 2023 17:05

Merge branch 'main' into maddy/replaceReadAsync

6fc306d

MaddyDev merged commit cfb4b85 into main Nov 1, 2023

MaddyDev deleted the maddy/replaceReadAsync branch November 1, 2023 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace read async with read of SqlReader for perf #904

replace read async with read of SqlReader for perf #904

MaddyDev commented Aug 9, 2023

Charles-Gagnon commented Aug 9, 2023

cheenamalhotra Aug 9, 2023

Charles-Gagnon Aug 9, 2023

Charles-Gagnon Aug 10, 2023

Charles-Gagnon Aug 10, 2023

lucyzhang929 Aug 11, 2023 •

edited

Loading

Charles-Gagnon Aug 11, 2023

chlafreniere commented Aug 11, 2023

Charles-Gagnon commented Aug 11, 2023

GeeSuth commented Sep 18, 2023

		@@ -112,7 +112,7 @@ private async Task<bool> GetNextRowAsync()
		this._reader = await command.ExecuteReaderAsync();

replace read async with read of SqlReader for perf #904

replace read async with read of SqlReader for perf #904

Conversation

MaddyDev commented Aug 9, 2023

Charles-Gagnon commented Aug 9, 2023

cheenamalhotra Aug 9, 2023

Choose a reason for hiding this comment

Charles-Gagnon Aug 9, 2023

Choose a reason for hiding this comment

Charles-Gagnon Aug 10, 2023

Choose a reason for hiding this comment

Charles-Gagnon Aug 10, 2023

Choose a reason for hiding this comment

lucyzhang929 Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Charles-Gagnon Aug 11, 2023

Choose a reason for hiding this comment

chlafreniere commented Aug 11, 2023

Charles-Gagnon commented Aug 11, 2023

GeeSuth commented Sep 18, 2023

lucyzhang929 Aug 11, 2023 •

edited

Loading