Extend LeaseCollection allowed partition key paths to "\partitionKey" to support gremlin accounts #158

CPrashanth · 2021-03-18T18:41:46Z

scenario-

create a gremlin cosmosdb account.
Cosmosdb exposes both gremlin and sql api for the account.
create a collection and read/write data using sql api. Note not using graph api.
setup changefeedprocessor library for sql collection created in step 3

Expected: Just like the following works for a collection with sql api on gremlin account, expectation is changefeedprocessor also works and doesnt special case gremlin accounts which the below apis dont do.

Gets/Put using sql Api
ChangeFeed Pull Api
ChangeFeed Azure Functions

Actual: ChangeFeedProcessor requires LeaseCollection to be created with "id" PartitionKey which is not supported on Gremlin Accounts. We get the following error when the lease collection is created. "“Microsoft.Azure.Documents.DocumentClientException: Partition key path /id is invalid for Gremlin API. The path cannot be '/id', '/label' or a nested path such as '/key/path'.""

Fix:

Allow lease collection with path partitionKey and id
Create another property partitionKey in the DocumentServiceLease class
by default, don't serialize or fill in the property value.
only if the lease collection partition key path is "partitionKey", fill in the property value.
the above is achieved by creating another RequestOptionsFactory class for partitionKey, which stamps the lease object with the property value

Tested:
Ran UT, IT locally with cosmosdb emulator in gremlin mode.
ran ut, it against azure sql account and azure graph account and verified the cases passed

Closes #156

ghost · 2021-03-18T18:42:00Z

All CLA requirements met.

CPrashanth · 2021-03-18T18:46:30Z

Fix the below issue
#156

ealsur · 2021-03-18T19:08:09Z

src/DocumentDB.ChangeFeedProcessor/LeaseManagement/DocumentServiceLease.cs

+        /// Gets or sets property to be used as partition key path for lease collections created in GremlinAccounts
+        /// </summary>
+        [JsonProperty("gremlincompatid")]
+        public string GremlinCompatId


What if the user has a property with this name? Wouldn't be better for the user to specify the pk they want to use?

Note the problem you are specifying, exists even today before this PR. Default implementation assumes it can be only "id"

This is to address the default scenario where end user is using default lease management from this library.
In this case, property names are controlled by this class and there would be no conflict.
In their custom lease management solution, it doesnt hit this path as I understand

I understand, what I'm saying is that this fails with the same assumption that currently happens with /id. Giving the user the choice would be better suited.

Let me give an example and you can tell me how it could work so that I can understand the proposal better
lets say I choose FooId as my partitionKey for the lease collection and created the collection. now when I pass it down to this library, what should the library do to the DocumentServiceLease class so that it can stamp the objects with FooId property.

Will look at your reply for above example, but I feel that what you are proposing would not be possible as long as lease implementation and its class definition is controlled by this library.
Unless you are talking reflection and dynamically adding properties. I am not sure whether we want that kind of complexity, only reason customers complain about id as current partition property is because it doesnt work on gremlin.

Ideally this library should create the lease collection since it controls definition of DocumentServiceLease and stores it in the lease collection. That way customer doesn't have to deal with identifying partitionkeyPath looking at the internal implementation of this class.

mkolt

can you please provide better description: explain the problem and outline the design (and why this is going to work).

mkolt · 2021-03-19T16:44:31Z

The description is still not clear. Can we have description that OSS user with minimum knowledge of CFP can reason about? Besides, scenario is not clear as well.

why does scenario has "create a sql collection and write data into it (Cosmosdb allows it)"? How is this related to the scenario?
there is no step 35.
"since changefeedprocessor library uses "id" as partition key" -- CFP library doesn't use partition keys. PKs are provided by either monitoring collection or lease collection.
CFP requires lease collection, if partitioned, to have PK as /id. If this is the issue you are trying to address? Why is this a problem for reading change feed when monitored collection is gremlin?
create alias property GremlinCompatId for Id field. Allow gremlingcompatid in addition to id field as partition key path. Where is that getting created and how does it help? What is the workflow when this is used?

Also for CFP we should not special case gremlin. If we do something, it should be generic and extensible.

src/DocumentDB.ChangeFeedProcessor/LeaseManagement/DocumentServiceLeaseStore.cs

mkolt · 2021-03-19T17:58:26Z

src/DocumentDB.ChangeFeedProcessor/LeaseManagement/DocumentServiceLease.cs

+        /// Gets or sets property to be used as partition key path for lease collections created in GremlinAccounts
+        /// </summary>
+        [JsonProperty(GremlinCompatIdPropertyName)]
+        public string GremlinCompatId


We should not do that by default. Majority of lease collections do not need this overhead.

Can you suggest, what has to be done. I tried to interpret this comment below but not sure whether I got it right?

I can change gremlincompatId into LeaseId property name so that its not specific to gremlin and hopefully LeaseId doesnt impact any of other types of cosmosdb account. if you have a way where we can change Id into LeaseId thats ideal considering already collections are provisioned with partitionKey as "Id".

When you are saying its overhead, what's the concern. Is it processing cost, storage cost? clone of Id field is not costly right.
Are you saying we should create a derived class DocumentServiceLeaseV2 and add this property LeaseId there. That would be major refactoring to have parallel classes or if else within existing classes e.g DocumentServiceLeaseStoreManager.cs to conditionally create DocumentServiceLease vs DocumentServiceLeaseV2 based on user input. Overhead cost of additional string is cheaper to this entire effort.

One way I think about is using something like (existing) IRequestOptionsFactory, that would use something like Lease.PartitionKey or Lease.AlternatePartitionKey and all operations would append PK to request options and update lease doc for lease operations. That would only be used when CFP Builder explicitly opts-in for using alternate PK for lease collection. In fact, if we go this way, it should not be much more work than just supporting any property used as PK when lease collection was created.

is it something along this line, I will fix the names later
internal class PartitionedByAlternatePKCollectionRequestOptionsFactory : IRequestOptionsFactory
{
public RequestOptions CreateRequestOptions(ILease lease) => new RequestOptions { PartitionKey = new PartitionKey(lease.AlternatePK) };

public FeedOptions CreateFeedOptions() => new FeedOptions { EnableCrossPartitionQuery = true }; }

then instantiate that class in CFBuilder based on some flag.

wont this mean the following:

ILease interface has to be updated with a property name "AlternatePK"

if somebody implemented their custom lease management, they need to unnecessarily add this property. Is It Ok?

the moment I touch ILease, i need to update DocumentServiceLease to have that property. so whether I use new RequestOptionsFactory, lease document will still contain the "AlternatePK". so what does it help in?

if somebody created a lease collection with "FooId" as partitionkey. Still not clear, how I can make it work without touching ILease interface to have "FooId" as a property name. Or is this not a ask at all from you and Matias?

ILease doesn't need to be touched. DocumentServiceLease is only specific to CosmosDB service, e.g. redis or in memory lease implementation wouldn't have partition key concept. I think with these changes we can add properties to document service lease either by adding to the class itself or by calling SetProerty via NewtonsoftJson as you did in your ealier changes. I would prefer the latter approach with use controlling what PK they want to use (in the upper stack we would call API to get PK definition from the lease collection at startup).

can you check this pr - https://github.com/Azure/azure-documentdb-changefeedprocessor-dotnet/pull/159/files
and see if that aligns with what you are proposing
this is rough change and ignore the test cases, I haven't fixed or authored new ones yet

mkolt · 2021-04-26T02:42:18Z

src/DocumentDB.ChangeFeedProcessor/LeaseManagement/DocumentServiceLease.cs

+        /// This property name is compatible to both GremlinAccounts and SqlAccounts
+        /// </summary>
+        [JsonProperty(LeasePartitionKeyPropertyName)]
+        public string LeasePartitionKey { get; set; }


Can we name it as just PartitionKey. This class is already a lease.

mkolt · 2021-04-26T02:43:31Z

src/DocumentDB.ChangeFeedProcessor/LeaseManagement/DocumentServiceLease.cs

@@ -14,6 +14,9 @@ namespace Microsoft.Azure.Documents.ChangeFeedProcessor.LeaseManagement
    [Serializable]
    internal class DocumentServiceLease : ILease, ILeaseAcquireReasonProvider
    {
+        internal const string IdPropertyName = "id";
+        internal const string LeasePartitionKeyPropertyName = "leasepk";


it's already a lease. Can we have this as "pk" or "partitionKey", I would prefer the latter as it more clear?

mkolt · 2021-04-26T02:45:49Z

src/DocumentDB.ChangeFeedProcessor/ChangeFeedProcessorBuilder.cs

-                    (collection.PartitionKey.Paths.Count != 1 || collection.PartitionKey.Paths[0] != "/id"))
+
+                if (isPartitioned &&
+                    (collection.PartitionKey.Paths.Count != 1 || !(collection.PartitionKey.Paths[0].Equals($"/{DocumentServiceLease.IdPropertyName}", StringComparison.OrdinalIgnoreCase) ||


please break too long lines. Normally 120 chars.

mkolt · 2021-04-26T02:47:23Z

src/DocumentDB.ChangeFeedProcessor/ChangeFeedProcessorBuilder.cs

                {
-                    throw new ArgumentException("The lease collection, if partitioned, must have partition key equal to id.");
+                    throw new ArgumentException($"The lease collection, if partitioned, must have partition key equal to {DocumentServiceLease.IdPropertyName} or {DocumentServiceLease.LeasePartitionKeyPropertyName}.");


more correct to say /id or /partitionKey

mkolt · 2021-04-26T02:50:17Z

src/DocumentDB.ChangeFeedProcessor/ChangeFeedProcessorBuilder.cs

+
+                if (isPartitioned &&
+                    (collection.PartitionKey.Paths.Count != 1 || !(collection.PartitionKey.Paths[0].Equals($"/{DocumentServiceLease.IdPropertyName}", StringComparison.OrdinalIgnoreCase) ||
+                                                                   collection.PartitionKey.Paths[0].Equals($"/{DocumentServiceLease.LeasePartitionKeyPropertyName}", StringComparison.OrdinalIgnoreCase))))


Can't use OrdinalIgnoreCase. JSON is case-sensitive. Just compare using "!=" . If using .Equals also check that Paths[0] is not null.

mkolt · 2021-04-26T02:51:36Z

src/DocumentDB.ChangeFeedProcessor/ChangeFeedProcessorBuilder.cs

+                    // we allow only id or leasePk partitioning so check only flag
+                    requestOptionsFactory = collection.PartitionKey.Paths[0].Equals($"/{DocumentServiceLease.IdPropertyName}", StringComparison.OrdinalIgnoreCase) ?
+                                            (IRequestOptionsFactory)new PartitionedByIdCollectionRequestOptionsFactory() :
+                                            (IRequestOptionsFactory)new PartitionedByLeasePkCollectionRequestOptionsFactory();


I don't think cast to interface is needed. Is it?

gives cs8370 error. conditional target type expression available in c# 9.0

mkolt · 2021-04-26T02:53:54Z

...B.ChangeFeedProcessor/LeaseManagement/PartitionedByLeasePkCollectionRequestOptionsFactory.cs

+            var documentServiceLease = lease as DocumentServiceLease;
+            if (documentServiceLease == null)
+                throw new ArgumentException("lease is not of type DocumentServiceLease");
+            if (string.IsNullOrWhiteSpace(documentServiceLease.LeasePartitionKey))


This check is technically OK to have as we control setting PK value in the lease document. But in general null/empty PK value is valid.

gets, delete of lease were failing if PK value is null. Searches were working fine

mkolt · 2021-04-26T02:55:25Z

...B.ChangeFeedProcessor/LeaseManagement/PartitionedByLeasePkCollectionRequestOptionsFactory.cs

+    using Microsoft.Azure.Documents.Client;
+
+    /// <summary>
+    /// Used to create request options for partitioned lease collections, when partition key is defined as /leasepk.


/leasepk: please don't forget to adjust this to latest version of what we decided.

src/DocumentDB.ChangeFeedProcessor/ChangeFeedProcessorBuilder.cs

mkolt · 2021-04-27T20:24:07Z

src/DocumentDB.ChangeFeedProcessor/ChangeFeedProcessorBuilder.cs

+                if (isPartitioned)
+                {
+                    // we allow only id or leasePk partitioning so check only flag
+                    requestOptionsFactory = collection.PartitionKey.Paths[0] != PartitionKeyPkPathName ?


nit: better to check for == rather than !=. This will avoid extra churn in future.

...ngeFeedProcessor/LeaseManagement/PartitionedByPartitionKeyCollectionRequestOptionsFactory.cs

mkolt · 2021-04-27T20:35:42Z

src/DocumentDB.ChangeFeedProcessor/LeaseManagement/DocumentServiceLease.cs

+        /// This is clone of existing Id property to maintain backward compat.
+        /// This property name is compatible to both GremlinAccounts and SqlAccounts
+        /// </summary>
+        [JsonProperty(LeasePartitionKeyPropertyName, NullValueHandling= NullValueHandling.Ignore)]


nit: spaces around '='

mkolt · 2021-04-27T20:42:02Z

src/DocumentDB.ChangeFeedProcessor.IntegrationTests/LeasePkLeaseCollectionTests.cs

+    public class LeasePkLeaseCollectionTests : StaticCollectionTests
+    {
+        public LeasePkLeaseCollectionTests() :
+            base(true,true)


nit: space after comma

mkolt · 2021-04-27T20:43:23Z

@ealsur can you please review last iteration?

…azure-documentdb-changefeedprocessor-dotnet into gremlinSqlCollection

src/DocumentDB.ChangeFeedProcessor/LeaseManagement/DocumentServiceLeaseStoreManager.cs

src/DocumentDB.ChangeFeedProcessor/LeaseManagement/DocumentServiceLeaseStore.cs

src/DocumentDB.ChangeFeedProcessor.UnitTests/LeaseManagement/DocumentServiceLeaseTests.cs

...ngeFeedProcessor/LeaseManagement/PartitionedByPartitionKeyCollectionRequestOptionsFactory.cs

ealsur · 2021-04-28T16:27:29Z

/azp run

azure-pipelines · 2021-04-28T16:27:34Z

No pipelines are associated with this pull request.

ealsur · 2021-04-28T16:31:56Z

/azp run

azure-pipelines · 2021-04-28T16:32:11Z

Azure Pipelines successfully started running 2 pipeline(s).

ealsur · 2021-04-28T20:24:58Z

/azp run

azure-pipelines · 2021-04-28T20:25:13Z

Azure Pipelines successfully started running 2 pipeline(s).

ealsur · 2021-05-01T01:50:16Z

/azp run

azure-pipelines · 2021-05-01T01:50:31Z

Azure Pipelines successfully started running 2 pipeline(s).

support gremlin accounts

f0d9202

fix ut

9303d4f

ealsur reviewed Mar 18, 2021

View reviewed changes

add compat tests

2c77bf0

CPrashanth marked this pull request as ready for review March 18, 2021 21:08

mkolt requested changes Mar 18, 2021

View reviewed changes

CPrashanth requested a review from mkolt March 18, 2021 22:20

CPrashanth marked this pull request as draft March 19, 2021 00:44

fix store init documents to contain gremlincompatid

1ce05c8

CPrashanth marked this pull request as ready for review March 19, 2021 05:14

mkolt reviewed Mar 19, 2021

View reviewed changes

src/DocumentDB.ChangeFeedProcessor/LeaseManagement/DocumentServiceLeaseStore.cs Outdated Show resolved Hide resolved

mkolt reviewed Mar 19, 2021

View reviewed changes

change gremlincompatid to leaseid

8e737ed

vidyavardhan approved these changes Mar 22, 2021

View reviewed changes

CPrashanth requested review from ealsur and mkolt March 23, 2021 20:16

Prashanth Chandrasekaran added 3 commits April 22, 2021 16:34

enabling leasepk as new partition key

522b67a

enabling leasepk as new partition key

f165e83

fix merge conflict

38eaa9b

mkolt reviewed Apr 26, 2021

View reviewed changes

Prashanth Chandrasekaran added 2 commits April 26, 2021 22:24

fix mc

f5c51e2

fix mc

a230873

mkolt reviewed Apr 27, 2021

View reviewed changes

src/DocumentDB.ChangeFeedProcessor/ChangeFeedProcessorBuilder.cs Show resolved Hide resolved

mkolt reviewed Apr 27, 2021

View reviewed changes

...ngeFeedProcessor/LeaseManagement/PartitionedByPartitionKeyCollectionRequestOptionsFactory.cs Show resolved Hide resolved

mkolt reviewed Apr 27, 2021

View reviewed changes

Prashanth Chandrasekaran added 3 commits April 27, 2021 14:09

using == instead of != for path comparison

8e4fabb

using == instead of != for path comparison

3f0e2ce

Merge branch 'gremlinSqlCollection' of https://github.com/CPrashanth/…

4f0c802

…azure-documentdb-changefeedprocessor-dotnet into gremlinSqlCollection

ealsur reviewed Apr 28, 2021

View reviewed changes

Merge branch 'master' into gremlinSqlCollection

b33120d

ealsur approved these changes Apr 28, 2021

View reviewed changes

ealsur mentioned this pull request Apr 28, 2021

On Gremlin accounts, changefeed collection creation fails with Partition key path /id is invalid for Gremlin API. Azure/azure-cosmos-dotnet-v3#2291

Closed

made monitored collection partition key path conditional

4938e25

mkolt approved these changes Apr 30, 2021

View reviewed changes

CPrashanth closed this Apr 30, 2021

CPrashanth reopened this Apr 30, 2021

CPrashanth changed the title ~~support gremlin accounts~~ Extend LeaseCollection allowed partition key paths to "\partitionKey" to support gremlin accounts May 3, 2021

mkolt merged commit f43e6b3 into Azure:master May 3, 2021

ealsur mentioned this pull request May 21, 2021

Change Feed Processor: Adds support for Graph API accounts Azure/azure-cosmos-dotnet-v3#2491

Merged

1 task

Extend LeaseCollection allowed partition key paths to "\partitionKey" to support gremlin accounts #158

Extend LeaseCollection allowed partition key paths to "\partitionKey" to support gremlin accounts #158

Conversation

CPrashanth commented Mar 18, 2021 • edited Loading

ghost commented Mar 18, 2021 • edited by ghost Loading

CPrashanth commented Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPrashanth Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

mkolt left a comment

Choose a reason for hiding this comment

mkolt commented Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkolt Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkolt Mar 26, 2021 • 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

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

mkolt commented Apr 27, 2021

ealsur commented Apr 28, 2021

azure-pipelines bot commented Apr 28, 2021

ealsur commented Apr 28, 2021

azure-pipelines bot commented Apr 28, 2021

ealsur commented Apr 28, 2021

azure-pipelines bot commented Apr 28, 2021

ealsur commented May 1, 2021

azure-pipelines bot commented May 1, 2021

CPrashanth commented Mar 18, 2021 •

edited

Loading

ghost commented Mar 18, 2021 •

edited by ghost

Loading

CPrashanth commented Mar 18, 2021 •

edited

Loading

CPrashanth Mar 19, 2021 •

edited

Loading

mkolt commented Mar 19, 2021 •

edited

Loading

mkolt Mar 25, 2021 •

edited

Loading

mkolt Mar 26, 2021 •

edited

Loading