-
Notifications
You must be signed in to change notification settings - Fork 22
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.
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
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
Changes from 8 commits
f0d9202
9303d4f
2c77bf0
1ce05c8
8e737ed
522b67a
f165e83
38eaa9b
8ef8024
bb5e1fd
3e14295
f5c51e2
a230873
8e4fabb
3f0e2ce
4f0c802
b33120d
4938e25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
//---------------------------------------------------------------- | ||
// Copyright (c) Microsoft Corporation. Licensed under the MIT license. | ||
//---------------------------------------------------------------- | ||
|
||
namespace Microsoft.Azure.Documents.ChangeFeedProcessor.IntegrationTests | ||
{ | ||
using Xunit; | ||
|
||
/// <summary> | ||
/// To truly test this class, run emulator with /EnableGremlinEndpoint | ||
/// </summary> | ||
[Trait("Category", "Integration")] | ||
[Collection("Integration tests")] | ||
public class LeasePkLeaseCollectionTests : StaticCollectionTests | ||
{ | ||
public LeasePkLeaseCollectionTests() : | ||
base(true,true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: space after comma There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
{ | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ namespace Microsoft.Azure.Documents.ChangeFeedProcessor.UnitTests.LeaseManagemen | |
using System.IO; | ||
using System.Runtime.Serialization.Formatters.Binary; | ||
using Microsoft.Azure.Documents.ChangeFeedProcessor.LeaseManagement; | ||
using Microsoft.Azure.Documents.ChangeFeedProcessor.PartitionManagement; | ||
using Newtonsoft.Json; | ||
using Xunit; | ||
|
||
[Trait("Category", "Gated")] | ||
|
@@ -55,6 +57,7 @@ public void ValidateProperties() | |
Assert.Equal(timestamp, lease.Timestamp); | ||
Assert.Equal(value, lease.Properties[key]); | ||
Assert.Equal(etag, lease.ConcurrencyToken); | ||
Assert.Null(lease.LeasePartitionKey); | ||
} | ||
|
||
[Fact] | ||
|
@@ -63,6 +66,7 @@ public void ValidateSerialization_AllFields() | |
DocumentServiceLease originalLease = new DocumentServiceLease | ||
{ | ||
Id = "id", | ||
LeasePartitionKey="leasePk", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's not modify existing tests but add new tests for the case when lease collection is partitioned by partitionKey. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
ETag = "etag", | ||
PartitionId = "0", | ||
Owner = "owner", | ||
|
@@ -80,6 +84,7 @@ public void ValidateSerialization_AllFields() | |
var lease = (DocumentServiceLease)formatter.Deserialize(stream2); | ||
|
||
Assert.Equal(originalLease.Id, lease.Id); | ||
Assert.Equal(originalLease.LeasePartitionKey, lease.LeasePartitionKey); | ||
Assert.Equal(originalLease.ETag, lease.ETag); | ||
Assert.Equal(originalLease.PartitionId, lease.PartitionId); | ||
Assert.Equal(originalLease.Owner, lease.Owner); | ||
|
@@ -102,12 +107,121 @@ public void ValidateSerialization_NullFields() | |
var lease = (DocumentServiceLease)formatter.Deserialize(stream2); | ||
|
||
Assert.Null(lease.Id); | ||
Assert.Null(lease.LeasePartitionKey); | ||
Assert.Null(lease.ETag); | ||
Assert.Null(lease.PartitionId); | ||
Assert.Null(lease.Owner); | ||
Assert.Null(lease.ContinuationToken); | ||
Assert.Equal(new DocumentServiceLease().Timestamp, lease.Timestamp); | ||
Assert.Empty(lease.Properties); | ||
} | ||
|
||
|
||
#region Compat_Tests | ||
// this class doesnt contain LeaseId property | ||
|
||
[Serializable] | ||
class DocumentServiceLeaseV1 | ||
{ | ||
private static readonly DateTime UnixStartTime = new DateTime(1970, 1, 1, 0, 0, 0, 0, DateTimeKind.Utc); | ||
|
||
[JsonProperty("id")] | ||
public string Id { get; set; } | ||
|
||
[JsonProperty("_etag")] | ||
public string ETag { get; set; } | ||
|
||
[JsonProperty("PartitionId")] | ||
public string PartitionId { get; set; } | ||
|
||
[JsonProperty("Owner")] | ||
public string Owner { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets the current value for the offset in the stream. | ||
/// </summary> | ||
[JsonProperty("ContinuationToken")] | ||
public string ContinuationToken { get; set; } | ||
|
||
[JsonIgnore] | ||
public DateTime Timestamp | ||
{ | ||
get { return this.ExplicitTimestamp ?? UnixStartTime.AddSeconds(this.TS); } | ||
set { this.ExplicitTimestamp = value; } | ||
} | ||
|
||
[JsonIgnore] | ||
public string ConcurrencyToken => this.ETag; | ||
|
||
[JsonProperty("properties")] | ||
public Dictionary<string, string> Properties { get; set; } = new Dictionary<string, string>(); | ||
|
||
[JsonIgnore] | ||
public LeaseAcquireReason AcquireReason { get; set; } | ||
|
||
[JsonProperty("timestamp")] | ||
private DateTime? ExplicitTimestamp { get; set; } | ||
|
||
[JsonProperty("_ts")] | ||
private long TS { get; set; } | ||
|
||
} | ||
|
||
[Fact] | ||
public void ValidateBackwardCompat_OldLeaseFormat() | ||
{ | ||
|
||
DocumentServiceLeaseV1 originalLease = new DocumentServiceLeaseV1 | ||
{ | ||
Id = "id", | ||
ETag = "etag", | ||
PartitionId = "0", | ||
Owner = "owner", | ||
ContinuationToken = "continuation", | ||
Timestamp = DateTime.Now - TimeSpan.FromSeconds(5), | ||
Properties = new Dictionary<string, string> { { "key", "value" } } | ||
}; | ||
|
||
var serializedV1Lease = JsonConvert.SerializeObject(originalLease); | ||
var lease = JsonConvert.DeserializeObject<DocumentServiceLease>(serializedV1Lease); | ||
|
||
Assert.Equal(originalLease.Id, lease.Id); | ||
Assert.Equal(null, lease.LeasePartitionKey); | ||
Assert.Equal(originalLease.ETag, lease.ETag); | ||
Assert.Equal(originalLease.PartitionId, lease.PartitionId); | ||
Assert.Equal(originalLease.Owner, lease.Owner); | ||
Assert.Equal(originalLease.ContinuationToken, lease.ContinuationToken); | ||
Assert.Equal(originalLease.Timestamp, lease.Timestamp); | ||
Assert.Equal(originalLease.Properties["key"], lease.Properties["key"]); | ||
} | ||
|
||
[Fact] | ||
public void ValidateForwardCompat_OldLeaseFormat() | ||
{ | ||
|
||
DocumentServiceLease originalLease = new DocumentServiceLease | ||
{ | ||
Id = "id", | ||
ETag = "etag", | ||
PartitionId = "0", | ||
Owner = "owner", | ||
ContinuationToken = "continuation", | ||
Timestamp = DateTime.Now - TimeSpan.FromSeconds(5), | ||
Properties = new Dictionary<string, string> { { "key", "value" } } | ||
}; | ||
|
||
var serializedLease = JsonConvert.SerializeObject(originalLease); | ||
var lease = JsonConvert.DeserializeObject<DocumentServiceLeaseV1>(serializedLease); | ||
|
||
Assert.Equal(originalLease.Id, lease.Id); | ||
Assert.Equal(originalLease.ETag, lease.ETag); | ||
Assert.Equal(originalLease.PartitionId, lease.PartitionId); | ||
Assert.Equal(originalLease.Owner, lease.Owner); | ||
Assert.Equal(originalLease.ContinuationToken, lease.ContinuationToken); | ||
Assert.Equal(originalLease.Timestamp, lease.Timestamp); | ||
Assert.Equal(originalLease.Properties["key"], lease.Properties["key"]); | ||
} | ||
#endregion | ||
CPrashanth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -374,7 +374,7 @@ public async Task<IChangeFeedProcessor> BuildAsync() | |
|
||
await this.InitializeCollectionPropertiesForBuildAsync().ConfigureAwait(false); | ||
|
||
ILeaseStoreManager leaseStoreManager = await this.GetLeaseStoreManagerAsync(this.leaseCollectionLocation, true).ConfigureAwait(false); | ||
ILeaseStoreManager leaseStoreManager = await this.GetLeaseStoreManagerAsync(this.leaseCollectionLocation).ConfigureAwait(false); | ||
IPartitionManager partitionManager = this.BuildPartitionManager(leaseStoreManager); | ||
return new ChangeFeedProcessor(partitionManager); | ||
} | ||
|
@@ -397,7 +397,7 @@ public async Task<IRemainingWorkEstimator> BuildEstimatorAsync() | |
|
||
await this.InitializeCollectionPropertiesForBuildAsync().ConfigureAwait(false); | ||
|
||
var leaseStoreManager = await this.GetLeaseStoreManagerAsync(this.leaseCollectionLocation, true).ConfigureAwait(false); | ||
var leaseStoreManager = await this.GetLeaseStoreManagerAsync(this.leaseCollectionLocation).ConfigureAwait(false); | ||
|
||
IRemainingWorkEstimator remainingWorkEstimator = new RemainingWorkEstimator( | ||
leaseStoreManager, | ||
|
@@ -461,8 +461,7 @@ private IPartitionManager BuildPartitionManager(ILeaseStoreManager leaseStoreMan | |
} | ||
|
||
private async Task<ILeaseStoreManager> GetLeaseStoreManagerAsync( | ||
DocumentCollectionInfo collectionInfo, | ||
bool isPartitionKeyByIdRequiredIfPartitioned) | ||
DocumentCollectionInfo collectionInfo) | ||
{ | ||
if (this.LeaseStoreManager == null) | ||
{ | ||
|
@@ -473,15 +472,26 @@ private async Task<ILeaseStoreManager> GetLeaseStoreManagerAsync( | |
collection.PartitionKey != null && | ||
collection.PartitionKey.Paths != null && | ||
collection.PartitionKey.Paths.Count > 0; | ||
if (isPartitioned && isPartitionKeyByIdRequiredIfPartitioned && | ||
(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) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please break too long lines. Normally 120 chars. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
collection.PartitionKey.Paths[0].Equals($"/{DocumentServiceLease.LeasePartitionKeyPropertyName}", StringComparison.OrdinalIgnoreCase)))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't use OrdinalIgnoreCase. JSON is case-sensitive. Just compare using "!=" . If using .Equals also check that Paths[0] is not null. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
{ | ||
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}."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. more correct to say /id or /partitionKey There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
|
||
var requestOptionsFactory = isPartitioned ? | ||
(IRequestOptionsFactory)new PartitionedByIdCollectionRequestOptionsFactory() : | ||
(IRequestOptionsFactory)new SinglePartitionRequestOptionsFactory(); | ||
IRequestOptionsFactory requestOptionsFactory = null; | ||
if (isPartitioned) | ||
{ | ||
// 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think cast to interface is needed. Is it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gives cs8370 error. conditional target type expression available in c# 9.0 |
||
} | ||
else | ||
{ | ||
requestOptionsFactory = (IRequestOptionsFactory)new SinglePartitionRequestOptionsFactory(); | ||
} | ||
|
||
string leasePrefix = this.GetLeasePrefix(); | ||
var leaseStoreManagerBuilder = new DocumentServiceLeaseStoreManagerBuilder() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's already a lease. Can we have this as "pk" or "partitionKey", I would prefer the latter as it more clear? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure |
||
|
||
private static readonly DateTime UnixStartTime = new DateTime(1970, 1, 1, 0, 0, 0, 0, DateTimeKind.Utc); | ||
|
||
public DocumentServiceLease() | ||
|
@@ -32,9 +35,17 @@ public DocumentServiceLease(DocumentServiceLease other) | |
this.Properties = other.Properties; | ||
} | ||
|
||
[JsonProperty("id")] | ||
[JsonProperty(IdPropertyName)] | ||
public string Id { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets property to be used as partition key path for lease collections. | ||
/// This is clone of existing Id property to maintain backward compat. | ||
/// This property name is compatible to both GremlinAccounts and SqlAccounts | ||
/// </summary> | ||
[JsonProperty(LeasePartitionKeyPropertyName)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make sure that when it's null, we don't serialize to JSON. I think that would be NullValueHandling property. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure |
||
public string LeasePartitionKey { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we name it as just PartitionKey. This class is already a lease. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure |
||
|
||
[JsonProperty("_etag")] | ||
public string ETag { get; set; } | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please break too long line