CosmosDB Trigger support #1677

ealsur · 2017-07-13T19:08:08Z

Starting the CosmosDB trigger integration following PR: Azure/azure-webjobs-sdk-extensions#244 to integrate with Azure Functions Script Host.

Working on adding E2E tests.

dnfclas · 2017-07-13T19:08:12Z

@ealsur,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

fabiocav · 2017-07-13T19:24:13Z

Thank you! Will take a look ASAP!

I've flagged your PR in the extensions repo so we can get a full CI build. Once that is merged, I'll push the new package to the MyGet feed to allow the CI tests to run as expected here.

mamaso · 2017-07-17T17:36:48Z

src/WebJobs.Script/Binding/DocumentDBScriptBindingProvider.cs

+
+                    if (!string.IsNullOrEmpty(leaseCollectionName))
+                    {
+                        attributeTrigger.LeaseCollectionName = leaseCollectionName;


how does this work if not all of these lease properties are set?

Also, it might be preferable to bundle all these lease properties in one object, i.e.

{ "type": "documentDbTrigger", "connection": ... "lease": { "collection": ..., "database": ..., "connection": ..., "options": { ... } } }

@mamaso By default, if no lease properties are set, the Lease database is the same as the databaseName attribute, the Lease colletion defaults to "leases" and the connection string is the same as the connection attribute. So it uses the same service, same database but different collection. We are also checking that an infinite loop is not created where both collections are the same.

I will rework the properties of the leases in a nested object in the next commit 😄

fabiocav

Changes look good! Just a question about the package version, as that may be problematic.

fabiocav · 2017-07-19T00:07:30Z

src/WebJobs.Script.Host/WebJobs.Script.Host.csproj

+    <Reference Include="Microsoft.Azure.Documents.ChangeFeedProcessor, Version=1.14.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
+      <HintPath>..\..\packages\Microsoft.Azure.DocumentDB.ChangeFeedProcessor.1.0.0\lib\net45\Microsoft.Azure.Documents.ChangeFeedProcessor.dll</HintPath>
+    </Reference>
+    <Reference Include="Microsoft.Azure.Documents.Client, Version=1.13.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Is the document client package version update required? This would change the version customers get with the `#r "Microsoft.Azure.Documents.Client" directive

@fabiocav Yes. The Change Feed API support within the SDK was introduced in version 1.13.1, that's the minimum version required and the one we are using, basically the restriction is >= 1.13.1 for Change Feed.

@fabiocav -- the version change is why we held off on getting this into Extensions in the last release. Ultimately, we'll need to move it forward but need to make sure the redirect is updated. There shouldn't be any breaking changes -- but we'll need to monitor the rollout to see if we get any error spikes.

brettsam · 2017-07-26T13:22:20Z

sample/CosmosDBTrigger-CSharp/run.csx

+using System;
+public static void Run(IReadOnlyList<Document> input)
+{
+    Console.WriteLine("Documents modified " + input.Count);


Using Console.WriteLine doesn't work from a function -- add a parameter ILogger logger and do a logger.LogInformation("") instead. That'll send the log to the file logs, App Inisights, etc.

Is TraceWriter ok too?

Yeah, either works. ILogger is our newer logger.

brettsam · 2017-07-26T13:24:09Z

src/WebJobs.Script.Host/WebJobs.Script.Host.csproj

+    <Reference Include="Microsoft.Azure.Documents.ChangeFeedProcessor, Version=1.14.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
+      <HintPath>..\..\packages\Microsoft.Azure.DocumentDB.ChangeFeedProcessor.1.0.0\lib\net45\Microsoft.Azure.Documents.ChangeFeedProcessor.dll</HintPath>
+    </Reference>
+    <Reference Include="Microsoft.Azure.Documents.Client, Version=1.13.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


@fabiocav -- the version change is why we held off on getting this into Extensions in the last release. Ultimately, we'll need to move it forward but need to make sure the redirect is updated. There shouldn't be any breaking changes -- but we'll need to monitor the rollout to see if we get any error spikes.

brettsam · 2017-07-26T13:31:10Z

src/WebJobs.Script/Binding/DocumentDBScriptBindingProvider.cs

-            Config.UseDocumentDB();
+            DocumentDBConfiguration documentDBConfiguration = new DocumentDBConfiguration();
+
+            JObject configSection = (JObject)Metadata.GetValue("documentDB", StringComparison.OrdinalIgnoreCase);


We're getting into a little of the weirdness of the "CosmosDB" vs "DocumentDB" naming conventions here. This global host.json setting isn't strictly required, is it? The lease options can be set in function.json for each function, if needed. I wonder if we just leave this off to keep things simpler and we can always re-add it in the future.

We were talking with Paul to do a renaming sometime in the future to CosmosDB in everything, but it would be a breaking change for current functions.

I could not make the Lease Options work on the function level. Mainly because, even if I parse it correctly, I cannot pass it to the Attribute because it's not a basic Type, so it doesn't work in:

[CosmosDBTrigger("ItemDb", "ItemCollection", LeaseOptions= new LeaseOptions(...))]

That is why I put it on the global host.json level.

Yeah makes sense -- let's go with this.

brettsam · 2017-07-26T13:37:02Z

src/WebJobs.Script/Binding/DocumentDBScriptBindingProvider.cs

+                    CosmosDBTriggerAttribute attributeTrigger = new CosmosDBTriggerAttribute(databaseName, collectionName);
+                    attributeTrigger.ConnectionString = connection;
+
+                    JObject leasesConfiguration = Context.GetMetadataValue<JObject>("lease");


I know @mamaso recommended we bundle these into a single object, but I seem to recall that the best approach is to have the properties match 1:1 with the attribute in extensions. Eventually, that'll let you remove this code completely and we'll be able to auto-map them. If you change the shape of the metadata (as compared to the attribute), that mapping is harder.

@MikeStall may have more input here.

brettsam · 2017-07-26T13:39:40Z

src/WebJobs.Script/Binding/DocumentDBScriptBindingProvider.cs

                {
-                    attribute = new DocumentDBAttribute(databaseName, collectionName);
+                    CosmosDBTriggerAttribute attributeTrigger = new CosmosDBTriggerAttribute(databaseName, collectionName);
+                    attributeTrigger.ConnectionString = connection;


I just noticed the difference in this name -- we should change this to ConnectionStringSetting. I think we're a little inconsistent with the naming, but we decided at some point that using Setting here made it more clear that it wasn't an actual connection string, but rather an app setting that points to a connection string.

Same with LeaseConnectionStringSetting (anything with the [AppSetting] attribute, really).

Done in next commit

brettsam · 2017-07-26T13:44:47Z

src/WebJobs.Script/Binding/DocumentDBScriptBindingProvider.cs

+                            attributeTrigger.LeaseConnectionString = leaseConnectionString.Value<string>();
+                        }
+                    }
+


Can you also add support for LeaseOptions here?

I could not make the Lease Options work on the function level. Mainly because, even if I parse it correctly, I cannot pass it to the Attribute because it's not a basic Type, so it doesn't work in:

[CosmosDBTrigger("ItemDb", "ItemCollection", LeaseOptions= new LeaseOptions(...))]

brettsam · 2017-07-26T13:46:28Z

test/WebJobs.Script.Tests.Integration/EndToEndTestFixture.cs

@@ -159,6 +162,56 @@ public virtual void Dispose()
            Host.Stop();
            Host.Dispose();
            ServiceBusQueueClient.Close();
+            if (DocumentClient != null)


You should be able to do DocumentClient?.Dispose() here. Saves a few lines :-)

I forget sometimes that I can use C# 6 here 😄

brettsam · 2017-07-26T13:49:12Z

test/WebJobs.Script.Tests.Integration/EndToEndTestFixture.cs

@@ -77,6 +78,8 @@ protected EndToEndTestFixture(string rootPath, string testId)

        public Microsoft.ServiceBus.Messaging.QueueClient ServiceBusQueueClient { get; private set; }

+        public Documents.Client.DocumentClient DocumentClient { get; private set; }


Can you add usings for Microsoft.Documents/Microsoft.Documents.Client so this (and some other lines) is a little shorter?

Done in next commit

brettsam · 2017-07-26T13:53:30Z

test/WebJobs.Script.Tests.Integration/EndToEndTestsBase.cs

+            // "AzureWebJobsDocumentDBConnectionString" -- the connection string to the account
+            await Fixture.InitializeDocumentClient();
+            bool collectionsCreated = await Fixture.CreateDocumentCollections();
+            var resultBlob = Fixture.TestOutputContainer.GetBlockBlobReference("completed");


minor: just to avoid confusion -- can you name this "cosmosdbtriggere2e-completed"? I know we use "completed" for some others, but it's nicer if it's clear.

Done in next commit

dnfclas added the cla-already-signed label Jul 13, 2017

fabiocav requested review from brettsam, fabiocav and mamaso July 13, 2017 19:24

mamaso reviewed Jul 17, 2017

View reviewed changes

fabiocav added the full-ci label Jul 19, 2017

fabiocav reviewed Jul 19, 2017

View reviewed changes

ealsur changed the title ~~DocumentDB Trigger support~~ CosmosDB Trigger support Jul 20, 2017

ealsur mentioned this pull request Jul 20, 2017

Adding CosmosDB Trigger Template Azure/azure-functions-templates#497

Merged

brettsam reviewed Jul 26, 2017

View reviewed changes

Adding Cosmos DB Trigger support to Script Host

28675ec

brettsam merged commit a7d1ba4 into Azure:dev Jul 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CosmosDB Trigger support #1677

CosmosDB Trigger support #1677

ealsur commented Jul 13, 2017 •

edited

Loading

dnfclas commented Jul 13, 2017

fabiocav commented Jul 13, 2017

mamaso Jul 17, 2017

ealsur Jul 18, 2017

fabiocav left a comment

fabiocav Jul 19, 2017

ealsur Jul 19, 2017 •

edited

Loading

brettsam Jul 26, 2017

brettsam Jul 26, 2017

ealsur Jul 26, 2017

brettsam Jul 26, 2017

brettsam Jul 26, 2017

brettsam Jul 26, 2017

ealsur Jul 26, 2017

brettsam Jul 26, 2017

brettsam Jul 26, 2017

brettsam Jul 26, 2017

ealsur Jul 26, 2017

brettsam Jul 26, 2017

ealsur Jul 26, 2017

brettsam Jul 26, 2017

ealsur Jul 26, 2017

brettsam Jul 26, 2017

ealsur Jul 26, 2017

brettsam Jul 26, 2017

ealsur Jul 26, 2017

		@@ -77,6 +78,8 @@ protected EndToEndTestFixture(string rootPath, string testId)

		public Microsoft.ServiceBus.Messaging.QueueClient ServiceBusQueueClient { get; private set; }

		public Documents.Client.DocumentClient DocumentClient { get; private set; }

CosmosDB Trigger support #1677

CosmosDB Trigger support #1677

Conversation

ealsur commented Jul 13, 2017 • edited Loading

dnfclas commented Jul 13, 2017

fabiocav commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiocav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ealsur Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ealsur commented Jul 13, 2017 •

edited

Loading

ealsur Jul 19, 2017 •

edited

Loading