Skip to content

Conversation

@AmeyaRele
Copy link
Contributor

No description provided.

@JatinSanghvi JatinSanghvi force-pushed the triggerbindings branch 3 times, most recently from 591638f to 565df26 Compare April 25, 2022 18:04
@JatinSanghvi JatinSanghvi changed the title Add Sql Trigger support Add SQL trigger support Apr 26, 2022
@JatinSanghvi JatinSanghvi force-pushed the triggerbindings branch 2 times, most recently from 4c9c2ed to bb95d88 Compare April 26, 2022 13:39
@Azure Azure deleted a comment from azure-pipelines bot May 18, 2022
Copy link
Contributor

@Charles-Gagnon Charles-Gagnon left a comment

Choose a reason for hiding this comment

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

Please add both unit and integration tests covering the functionality of this feature.

(also make sure you merge in latest from main and fix any build errors. We had to go back to netstandard2.0 from netstandard2.1 for bundle support which means some language features aren't available)

@AmeyaRele AmeyaRele force-pushed the triggerbindings branch 4 times, most recently from 25871ac to 24aac02 Compare June 20, 2022 09:58
@dzsquared
Copy link
Contributor

dzsquared commented Jun 23, 2022

reminder: this PR needs to be retargeted to a feature branch for trigger support and not merged to main

edit: this is already in a feature branch

Copy link
Contributor

@Charles-Gagnon Charles-Gagnon left a comment

Choose a reason for hiding this comment

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

Still a few comments to take a look at but overall looks good enough to me.

Although I want there to be a lot more debug logging done in all the parts. I'm not seeing any in SqlTableChangeMonitor for example. In general here are the things I'll be looking for :

  1. Message at the start of every task and sub task (Getting changes from table, Updating state tables, Getting list of changes, Got changed rows {....}) and so on. The goal should be to be able to tell exactly where something went wrong and what it was doing when an issue occurs, regardless of where that was.
  2. Log queries executed and how long they took
  3. Extra logging around any of the locking (include thread IDs if that isn't included by default)
  4. Make sure all errors are logged with any extra information that may be helpful in determining cause of the error

@lucyzhang929 @MaddyDev Please help make sure that we get as complete of a logging story as we can - we want to have as much as we can now before we give this to customers.

Also we should make sure to get basic usage telemetry set up - we can get some amount of stuff from the AF folks but that won't include people running locally so it'd be good to make sure we have a complete story ourselves as well. I'll follow up separately on my thoughts for that to get started.

@JatinSanghvi
Copy link
Contributor

Although I want there to be a lot more debug logging done in all the parts. I'm not seeing any in SqlTableChangeMonitor for example. In general here are the things I'll be looking for :

  1. Message at the start of every task and sub task (Getting changes from table, Updating state tables, Getting list of changes, Got changed rows {....}) and so on. The goal should be to be able to tell exactly where something went wrong and what it was doing when an issue occurs, regardless of where that was.
  2. Log queries executed and how long they took
  3. Extra logging around any of the locking (include thread IDs if that isn't included by default)
  4. Make sure all errors are logged with any extra information that may be helpful in determining cause of the error

Created #287 for this one.

@JatinSanghvi
Copy link
Contributor

JatinSanghvi commented Aug 8, 2022

Closing this pull request as it need not be merged soon and need not be reviewed further. When the triggerbindings branch needs to be merged into main branch, it can be either merged directly or through a new PR.

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.

7 participants