Added Change Feed #11692
Added Change Feed #11692
Conversation
seanmcc-msft
requested review from
amishra-dev,
amnguye,
chidozieononiwu,
danieljurek,
JoshLove-msft,
kasobol-msft,
mitchdenny,
tg-msft and
weshaggard
as code owners
|
TODO - Add more unit tests. |
| /// <summary> | ||
| /// Constructor for testing. Do not use. | ||
| /// </summary> | ||
| internal Shard( |
There was a problem hiding this comment.
These test-only constructors weird me out, but because these object are stateful, it's challenging to get Shard, Chunk, Segment, and Change Feed into the correct starting state for the smaller unit tests.
There was a problem hiding this comment.
I'd rather mock BlobContainerClient. Over time this might get quite out of sync from "prod" code leading to situation where we're not testing real scenarios.
| /// <summary> | ||
| /// BlobChangeFeedEvent. | ||
| /// </summary> | ||
| public class BlobChangeFeedEvent |
There was a problem hiding this comment.
I would call it BlobChangeEvent
There was a problem hiding this comment.
I think we should consider leaving this as BlobChangeFeedEvent because we've got the same schema for changes routed through EventGrid and we don't have a Track 2 API or name for server-side logging event info yet. BlobChangeFeedEvent is a little verbose, but I don't want to take a good name that we might want for the main Blobs package someday.
|
Here's the API view (slightly out of date) - https://apiview.dev/Assemblies/Review/85046278a8764a02a11d65ab219b91ae @tg-msft has recommendations for the public model names. |
| /// the shards indexes: 0 1 2 0 1. | ||
| /// </summary> | ||
| [Test] | ||
| public async Task GetPage() |
There was a problem hiding this comment.
@gapra-msft, here's my Segment round robin test.
|
@kasobol-msft, @tg-msft, @gapra-msft, could you take a look? |
|
|
||
| private async Task Initalize(bool async) | ||
| { | ||
| // Check if Change Feed has been abled for this account. |
| if (!changeFeedContainerExists) | ||
| { | ||
| //TODO improve this error message | ||
| throw new ArgumentException("Change Feed hasn't been enabled on this account, or is current being enabled."); |
| jsonMetaSegment = JsonDocument.Parse(blobDownloadInfo.Content); | ||
| } | ||
|
|
||
| //TODO what happens when _lastConsumable advances an hour? |
There was a problem hiding this comment.
Maybe we should just default end time to "now" when change feed is requested ?
There was a problem hiding this comment.
I think we just default to the last hour present in the Change Feed.
| //TODO what should we return here? Also do we really need to check this on every page? | ||
| if (_currentSegment.DateTime > _endTime) | ||
| { | ||
| return new BlobChangeFeedEventPage(); | ||
| } | ||
|
|
||
| //TODO what should we return here? Also do we really need to check this on every page? | ||
| if (_currentSegment.DateTime > _lastConsumable) | ||
| { | ||
| return new BlobChangeFeedEventPage(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Shouldn't this be end of iterable?
| ContinuationToken = continuationToken; | ||
| } | ||
|
|
||
| // public BlobChangeFeedEventPage(Response raw, List<GenericRecord> data) |
| //TODO figure out if this is right. | ||
| public bool HasNext() | ||
| { | ||
| if (!_isInitalized) |
There was a problem hiding this comment.
Should this trigger initialize instead? or throw? Good test for this assumption might be - fetch change log for account with zero activity.
There was a problem hiding this comment.
Now we that have factories, _isInitalized isn't a thing anymore in ChangeFeed, Segment, or Shard.
| /// <summary> | ||
| /// Constructor for testing. Do not use. | ||
| /// </summary> | ||
| internal Shard( |
There was a problem hiding this comment.
I'd rather mock BlobContainerClient. Over time this might get quite out of sync from "prod" code leading to situation where we're not testing real scenarios.
I am mocking the BlobContainerClient, I have that constructor to avoid super complicate test setups to mock the functionality of Shard.Initalize(). |
Maybe |
Would there be any concurrency or perf issues if we switched to static factory methods? Several Shards could be being initialized at the same time. Just for background, the Initialize() methods are separate from the constructors because I don't think we can make async calls in the constructors. |
|
@ljian3377, please let me know if you find anything else. |
|
I'm going to merge this, we can continue iterating in the 73 base branch. |
| cursor = JsonSerializer.Deserialize<ChangeFeedCursor>(continuation); | ||
| ValidateCursor(_containerClient, cursor); | ||
| startTime = cursor.CurrentSegmentCursor.SegmentTime; | ||
| endTime = cursor.EndTime; |
There was a problem hiding this comment.
Is it better if endTime = minDate(cursor.EndTime, endTime); to allow user specify a new endTime?
There was a problem hiding this comment.
Currently, I don't provide an overload for a user to provide both a cursor and end date. I think it's conceptually simpler to have separate overloads for cursor and start/end time.
No description provided.