Fix/1822 depends on atomicity group id v4 #1831
Conversation
KenitoInc
added
the
Ready for review
| if (id != null && !this.requestIdToAtomicGroupId.ContainsKey(id)) | ||
| { | ||
| this.requestIdToAtomicGroupId.Add(id, atomicityGroupId); | ||
| } |
There was a problem hiding this comment.
Rather than do an explicit ContainsKey check before calling Add, why not just do:
if (id != null)
{
this.requestIdToAtomicGroupId.TryAdd(id, atomicityGroupId);
} #Resolved
There was a problem hiding this comment.
Fixed #Resolved
There was a problem hiding this comment.
Should you use TryAdd(id) rather than Add(id) to avoid possible exceptions if the id already exists, or are we asserting that the id doesn't already exist? (same for below)
In reply to: 454179343 [](ancestors = 454179343)
There was a problem hiding this comment.
Never mind; I see that you moved to HashSet, which is perfect.
In reply to: 467600534 [](ancestors = 467600534,454179343)
| /// for reversed lookup. | ||
| /// </summary> | ||
| private Dictionary<string, IList<string>> atomicityGroupIdToRequestId = new Dictionary<string, IList<string>>(); | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Why do you need dictionaries, it looks like you just check to see if the dictionary contains the key, not the relationship between the id and the atomicity group. Can you just use a list? #Resolved
There was a problem hiding this comment.
I have changed it to a HashSet. #Resolved
| /// Dictionary for keeping track of each atomic group's member request id. This is optimization | ||
| /// for reversed lookup. | ||
| /// </summary> | ||
| private Dictionary<string, IList<string>> atomicityGroupIdToRequestId = new Dictionary<string, IList<string>>(); |
There was a problem hiding this comment.
atomicityGroupIdToRequestId [](start = 50, length = 27)
atomicityGroupIdToRequestId [](start = 50, length = 27)
Do you need a separate list for atomicity group versus ids, or can you simply look up to see if the id has already been specified? #Resolved
There was a problem hiding this comment.
Fixed #Resolved
KenitoInc
force-pushed
the
fix/1822-DependsOnAtomicityGroupIdV4
branch
from
902d05a
to
3a628bf
Compare
Issues
This pull request fixes issue #1822 .
Description
Moved validations from
ODataBatchWriterandODataBatchReaderto individual implementations ofODataJsonLightBatchWriter,ODataJsonLightBatchReader,ODataMultipartMixedBatchWriterandODataMultipartMixedBatchReaderChecklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.