Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DynamoDB Reminders load using GSI #7437

Merged
merged 4 commits into from
Jan 6, 2022
Merged

DynamoDB Reminders load using GSI #7437

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e83d62
DynamoDB Reminders load using GSI
michaeltdaniels Dec 10, 2021
d919e27
Update DynamoDBStorage.cs
michaeltdaniels Jan 6, 2022
7ca0ca7
Update DynamoDBReminderTable.cs
michaeltdaniels Jan 6, 2022
3bbf221
Update DynamoDBReminderTable.cs
ReubenBond Jan 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 31 additions & 14 deletions src/AWS/Orleans.Reminders.DynamoDB/Reminders/DynamoDBReminderTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,31 +146,48 @@ public async Task<ReminderTableData> ReadRows(GrainReference grainRef)
/// <summary>
/// Reads reminder table data for a given hash range.
/// </summary>
/// <param name="beginHash"></param>
/// <param name="endHash"></param>
/// <param name="begin"></param>
/// <param name="end"></param>
/// <returns> Return the RemiderTableData if the rows were read successfully </returns>
public async Task<ReminderTableData> ReadRows(uint beginHash, uint endHash)
public async Task<ReminderTableData> ReadRows(uint begin, uint end)
{
var expressionValues = new Dictionary<string, AttributeValue>
{
{ $":{SERVICE_ID_PROPERTY_NAME}", new AttributeValue(this.serviceId) },
{ $":Begin{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = beginHash.ToString() } },
{ $":End{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = endHash.ToString() } }
};
Dictionary<string, AttributeValue> expressionValues = null;

try
{
string expression = string.Empty;
if (beginHash < endHash)
List<ReminderEntry> records;

if (begin < end)
{
expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} > :Begin{GRAIN_HASH_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} <= :End{GRAIN_HASH_PROPERTY_NAME}";
expressionValues = new Dictionary<string, AttributeValue>
{
{ $":{SERVICE_ID_PROPERTY_NAME}", new AttributeValue(this.serviceId) },
{ $":Begin{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = unchecked(begin + 1).ToString() } },
{ $":End{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = end.ToString() } }
};
expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} BETWEEN :Begin{GRAIN_HASH_PROPERTY_NAME} AND :End{GRAIN_HASH_PROPERTY_NAME}";
Copy link
Member

Choose a reason for hiding this comment

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

Please correct me if I'm wrong, but I believe this changes the query semantics. It's supposed to be > Begin && <= End (non-inclusive lower bound, inclusive upper), but BETWEEN is equivalent to => Begin && <= End (inclusive bounds)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're correct. Since performing a dynamodb query with an AND is not possible, would it be sufficient to pass the end attribute value as end-1 and still use BETWEEN?

Copy link
Member

Choose a reason for hiding this comment

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

@ReubenBond is correct. The query semantics are changed. I've overlooked this. The original one was correct. Are you sure this passed the unit tests? I remember the reminder tests being very nitpick about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed an unrelated change in DynamoDBStorage that has now been corrected.
AWUtils.Tests.RemindersTest passes with the questionable code as-is, and also if I change line 167 to (end - 1).ToString(), which I believe is the correct change to make.

Copy link
Member

@ReubenBond ReubenBond Jan 6, 2022 •
edited

Choose a reason for hiding this comment

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

Shouldn't it be begin + 1, rather than end - 1? The lower bound is supposed to be exclusive, not the upper bound.
It will work but please wrap the expression in unchecked(...) , since it's possible for it to overflow, just to make it extra obvious that it's expected behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thank you - vacation brain hasn't recovered yet.

Copy link
Member

Choose a reason for hiding this comment

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

No sweat, I'm in the same boat

Copy link
Member

Choose a reason for hiding this comment

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

Of course... it's impossible for both begin < end and begin == uint.MaxValue to be true at the same time, so the unchecked is entirely meaningless as no overflow is possible 😅

records = await this.storage.QueryAllAsync(this.options.TableName, expressionValues, expression, this.Resolve, SERVICE_ID_INDEX, consistentRead: false).ConfigureAwait(false);
}
else
{
expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND ({GRAIN_HASH_PROPERTY_NAME} > :Begin{GRAIN_HASH_PROPERTY_NAME} OR {GRAIN_HASH_PROPERTY_NAME} <= :End{GRAIN_HASH_PROPERTY_NAME})";
}
expressionValues = new Dictionary<string, AttributeValue>
{
{ $":{SERVICE_ID_PROPERTY_NAME}", new AttributeValue(this.serviceId) },
{ $":End{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = end.ToString() } }
};
expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} <= :End{GRAIN_HASH_PROPERTY_NAME}";
Copy link
Member

Choose a reason for hiding this comment

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

This query should be <= End || > Begin (since "end" is smaller than "begin", it's a wrap-around query).

Here's a graphical depiction of the two queries, where a circle represents an exclusive bound and a bar represents an inclusive bound:

image

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, the original query had this right: > Begin or <= End: it's a conjunction of two queries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally, this was performing a scan. I don't believe you can perform a query with the "or" clause, and thus I split the scan into two queries, then combine the results.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see now. That's an odd limitation, but nevertheless this seems fine.

records = await this.storage.QueryAllAsync(this.options.TableName, expressionValues, expression, this.Resolve, SERVICE_ID_INDEX, consistentRead: false).ConfigureAwait(false);

var records = await this.storage.ScanAsync(this.options.TableName, expressionValues, expression, this.Resolve).ConfigureAwait(false);
expressionValues = new Dictionary<string, AttributeValue>
{
{ $":{SERVICE_ID_PROPERTY_NAME}", new AttributeValue(this.serviceId) },
{ $":Begin{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = begin.ToString() } }
};
expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} > :Begin{GRAIN_HASH_PROPERTY_NAME}";
records.AddRange(await this.storage.QueryAllAsync(this.options.TableName, expressionValues, expression, this.Resolve, SERVICE_ID_INDEX, consistentRead: false).ConfigureAwait(false));

}

return new ReminderTableData(records);
}
Expand Down
14 changes: 8 additions & 6 deletions src/AWS/Shared/Storage/DynamoDBStorage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ public Task DeleteEntriesAsync(string tableName, IReadOnlyCollection<Dictionary<
{
if (Logger.IsEnabled(LogLevel.Trace)) Logger.Trace("Deleting {0} table entries", tableName);

if (toDelete == null) throw new ArgumentNullException("collection");
if (toDelete == null) throw new ArgumentNullException(nameof(toDelete));

if (toDelete.Count == 0)
return Task.CompletedTask;
Expand Down Expand Up @@ -460,16 +460,17 @@ public Task DeleteEntriesAsync(string tableName, IReadOnlyCollection<Dictionary<
/// <param name="indexName">In case a secondary index is used in the keyConditionExpression</param>
/// <param name="scanIndexForward">In case an index is used, show if the seek order is ascending (true) or descending (false)</param>
/// <param name="lastEvaluatedKey">The primary key of the first item that this operation will evaluate. Use the value that was returned for LastEvaluatedKey in the previous operation</param>
/// <param name="consistentRead">Determines the read consistency model. Note that if a GSI is used, this must be false.</param>
/// <returns>The collection containing a list of objects translated by the resolver function and the LastEvaluatedKey for paged results</returns>
public async Task<(List<TResult> results, Dictionary<string, AttributeValue> lastEvaluatedKey)> QueryAsync<TResult>(string tableName, Dictionary<string, AttributeValue> keys, string keyConditionExpression, Func<Dictionary<string, AttributeValue>, TResult> resolver, string indexName = "", bool scanIndexForward = true, Dictionary<string, AttributeValue> lastEvaluatedKey = null) where TResult : class
public async Task<(List<TResult> results, Dictionary<string, AttributeValue> lastEvaluatedKey)> QueryAsync<TResult>(string tableName, Dictionary<string, AttributeValue> keys, string keyConditionExpression, Func<Dictionary<string, AttributeValue>, TResult> resolver, string indexName = "", bool scanIndexForward = true, Dictionary<string, AttributeValue> lastEvaluatedKey = null, bool consistentRead = true) where TResult : class
{
try
{
var request = new QueryRequest
{
TableName = tableName,
ExpressionAttributeValues = keys,
ConsistentRead = true,
ConsistentRead = consistentRead,
KeyConditionExpression = keyConditionExpression,
Select = Select.ALL_ATTRIBUTES,
ExclusiveStartKey = lastEvaluatedKey
Expand Down Expand Up @@ -507,18 +508,19 @@ public Task DeleteEntriesAsync(string tableName, IReadOnlyCollection<Dictionary<
/// <param name="resolver">Function that will be called to translate the returned fields into a concrete type. This Function is only called if the result is != null and will be called for each entry that match the query and added to the results list</param>
/// <param name="indexName">In case a secondary index is used in the keyConditionExpression</param>
/// <param name="scanIndexForward">In case an index is used, show if the seek order is ascending (true) or descending (false)</param>
/// <param name="consistentRead">Determines the read consistency model. Note that if a GSI is used, this must be false.</param>
/// <returns>The collection containing a list of objects translated by the resolver function</returns>
public async Task<List<TResult>> QueryAllAsync<TResult>(string tableName, Dictionary<string, AttributeValue> keys,
string keyConditionExpression, Func<Dictionary<string, AttributeValue>, TResult> resolver,
string indexName = "", bool scanIndexForward = true) where TResult : class
string indexName = "", bool scanIndexForward = true, bool consistentRead = true) where TResult : class
{
List<TResult> resultList = null;
Dictionary<string, AttributeValue> lastEvaluatedKey = null;
do
{
List<TResult> results;
(results, lastEvaluatedKey) = await QueryAsync(tableName, keys, keyConditionExpression, resolver,
indexName, scanIndexForward, lastEvaluatedKey);
indexName, scanIndexForward, lastEvaluatedKey, consistentRead);
if (resultList == null)
{
resultList = results;
Expand Down Expand Up @@ -604,7 +606,7 @@ public Task PutEntriesAsync(string tableName, IReadOnlyCollection<Dictionary<str
{
if (Logger.IsEnabled(LogLevel.Trace)) Logger.Trace("Put entries {0} table", tableName);

if (toCreate == null) throw new ArgumentNullException("collection");
if (toCreate == null) throw new ArgumentNullException(nameof(toCreate));

if (toCreate.Count == 0)
return Task.CompletedTask;
Expand Down