refactor batch #20706
refactor batch #20706
Conversation
| public virtual void UpdateEntity<T>(T entity, Azure.ETag ifMatch, Azure.Data.Tables.TableUpdateMode mode = Azure.Data.Tables.TableUpdateMode.Merge) where T : class, Azure.Data.Tables.ITableEntity, new() { } | ||
| public virtual void UpsertEntity<T>(T entity, Azure.Data.Tables.TableUpdateMode mode = Azure.Data.Tables.TableUpdateMode.Merge) where T : class, Azure.Data.Tables.ITableEntity, new() { } | ||
| public virtual void UpdateEntity(Azure.Data.Tables.ITableEntity entity, Azure.ETag ifMatch, Azure.Data.Tables.TableUpdateMode mode = Azure.Data.Tables.TableUpdateMode.Merge) { } | ||
| public virtual void UpsertEntity(Azure.Data.Tables.ITableEntity entity, Azure.Data.Tables.TableUpdateMode mode = Azure.Data.Tables.TableUpdateMode.Merge) { } |
There was a problem hiding this comment.
How do we choose when to provide only singular overload and when the IEnumerable one?
There was a problem hiding this comment.
It's mostly an assumption that add is the common bulk scenario. We can always add more later if needed.
| { | ||
| throw new InvalidOperationException(TableConstants.ExceptionMessages.BatchIsEmpty); | ||
| } | ||
| if (transactionalBatch._submitted) |
There was a problem hiding this comment.
This seems to go against the batch being "just a model"
There was a problem hiding this comment.
Meant to remove that, thanks.
| throw new Exception($"Unexpected error. Unable to {nameof(RemoveEntityOperation)}"); | ||
| } | ||
| var itemsToRemove = new HashSet<string>(entities.Select(e => e.RowKey)); | ||
| var newQueue = new ConcurrentQueue<string>(); |
There was a problem hiding this comment.
ConcurrentQueue seems a bit heavyweight for what this method is doing.
| /// <param name="entities"></param> | ||
| /// <returns></returns> | ||
| /// <exception cref="NotImplementedException"></exception> | ||
| public bool RemoveEntityOperations(IEnumerable<ITableEntity> entities) |
There was a problem hiding this comment.
How would customers use the return value of this method?
There was a problem hiding this comment.
If, for some reason, we failed to remove the operations they could try again or inspect the current entities in the batch and make a decision about why it failed.
There was a problem hiding this comment.
What would the reason be if they passed a set of operations that a part of this batch?
There was a problem hiding this comment.
Perhaps some are not, in fact, a part of the batch.
This is modeled after List.Remove and Dictionary.Remove
There was a problem hiding this comment.
But those don't take enumerables.
There was a problem hiding this comment.
Are you suggesting it return void or that we return something more helpful, such as a list of the entities that could not be removed?
| private Guid _changesetGuid; | ||
| internal readonly Dictionary<string, BatchItem> _requestLookup = new(); | ||
| internal ConcurrentQueue<string> _requestMessages = new(); | ||
| internal List<BatchItem> _submittedMessageList; |
| @@ -32,12 +30,12 @@ internal TableBatchResponse(ConcurrentDictionary<string, (HttpMessage Message, R | |||
| /// <returns></returns> | |||
There was a problem hiding this comment.
nit: Since this is a public member, should we consider adding content for <returns> or removing the tag?
| internal readonly Dictionary<string, BatchItem> _requestLookup = new(); | ||
| internal ConcurrentQueue<string> _requestMessages = new(); | ||
| internal List<BatchItem> _submittedMessageList; | ||
| internal bool _submitted; |
There was a problem hiding this comment.
I think this may benefit from being volatile given its usage in the client when sending.
| private Guid _changesetGuid; | ||
| internal readonly Dictionary<string, BatchItem> _requestLookup = new(); | ||
| internal ConcurrentQueue<string> _requestMessages = new(); | ||
| internal List<BatchItem> _submittedMessageList; |
| { | ||
| throw new InvalidOperationException(TableConstants.ExceptionMessages.BatchCanOnlyBeSubmittedOnce); | ||
| } | ||
| transactionalBatch._submitted = true; |
There was a problem hiding this comment.
If this operation fails, doesn't this prevent retries? Should we consider unsetting in the catch block, or is the intent to create a new batch and use that?
| @@ -127,7 +127,7 @@ public async Task TablesTeardown() | |||
| /// <param name="partitionKeyValue">The partition key to create for the entity.</param> | |||
| /// <param name="count">The number of entities to create</param> | |||
| /// <returns></returns> | |||
| protected static List<TableEntity> CreateTableEntities(string partitionKeyValue, int count) | |||
| internal static List<TableEntity> CreateTableEntities(string partitionKeyValue, int count) | |||
There was a problem hiding this comment.
Should we consider refactoring into a helper class if we're calling outside of when the base class is used?
|
See #20723 for an alternate refactor. |
|
dupe of #20723 |
closes #20602